From 05bda743cd31d56a976914c7bcdec36384a3a805 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Mon, 23 Feb 2026 19:37:26 +0000 Subject: [PATCH 1/6] Added ScanServer property for allowed tables Added property that allows user to configure which tables are allowed to be scanned by clients at the ScanServer group level. Property defaults to allowing all non-accumulo namespace tables. Closes #6123 --- .../apache/accumulo/core/conf/Property.java | 7 + .../apache/accumulo/tserver/ScanServer.java | 101 +++++++- .../accumulo/tserver/ScanServerTest.java | 77 ++++-- .../test/ScanServerAllowedTablesIT.java | 238 ++++++++++++++++++ 4 files changed, 389 insertions(+), 34 deletions(-) create mode 100644 test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java b/core/src/main/java/org/apache/accumulo/core/conf/Property.java index 17edff8d2ea..90602ad6165 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java @@ -561,6 +561,13 @@ public enum Property { "The amount of time a scan reference is unused before its deleted from metadata table.", "2.1.0"), @Experimental + SSERV_SCAN_ALLOWED_TABLES("sserver.scan.allowed.tables.group.", null, PropertyType.PREFIX, + "A regular expression that determines which tables are allowed to be scanned for" + + " servers in the specified group. The property name should end with the scan server" + + " group and the property value should take into account the table namespace and name." + + " The default value disallows scans on tables in the accumulo namespace.", + "2.1.5"), + @Experimental SSERV_THREADCHECK("sserver.server.threadcheck.time", "1s", PropertyType.TIMEDURATION, "The time between adjustments of the thrift server thread pool.", "2.1.0"), // properties that are specific to tablet server behavior diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java index 83f57347b16..0d14f5bd3ae 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java @@ -45,6 +45,9 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.Condition; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -54,6 +57,7 @@ import org.apache.accumulo.core.conf.AccumuloConfiguration; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.core.conf.cluster.ClusterConfigParser; +import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.dataImpl.KeyExtent; import org.apache.accumulo.core.dataImpl.thrift.InitialMultiScan; import org.apache.accumulo.core.dataImpl.thrift.InitialScan; @@ -177,6 +181,8 @@ private TabletMetadataLoader(Ample ample) { } private static final Logger LOG = LoggerFactory.getLogger(ScanServer.class); + // Default pattern to allow scans on all tables not in accumulo namespace + private static final String DEFAULT_SCAN_ALLOWED_PATTERN = "^(?!accumulo\\.).*$"; protected ThriftScanClientHandler delegate; private UUID serverLockUUID; @@ -213,6 +219,9 @@ private TabletMetadataLoader(Ample ample) { private final String groupName; + private final ConcurrentHashMap allowedTables = new ConcurrentHashMap<>(); + private volatile String currentAllowedTableRegex; + public ScanServer(ScanServerOpts opts, String[] args) { super("sserver", opts, args); @@ -388,6 +397,7 @@ public void run() { } SecurityUtil.serverLogin(getConfiguration()); + updateAllowedTables(false); ServerAddress address = null; try { @@ -423,6 +433,7 @@ public void run() { Thread.sleep(1000); updateIdleStatus(sessionManager.getActiveScans().isEmpty() && tabletMetadataCache.estimatedSize() == 0); + updateAllowedTables(false); } catch (InterruptedException e) { LOG.info("Interrupt Exception received, shutting down"); gracefulShutdown(getContext().rpcCreds()); @@ -477,6 +488,79 @@ public void run() { } } + // Visible for testing + protected boolean isAllowed(TableId tid) { + boolean result = allowedTables.containsKey(tid); + if (!result) { + // Clear the cache and try again, maybe there + // is a race condition in table creation and + // scan + updateAllowedTables(true); + result = allowedTables.containsKey(tid); + } + return result; + } + + private synchronized void updateAllowedTables(boolean clearCache) { + + LOG.debug("Updating allowed tables for ScanServer"); + if (clearCache) { + context.clearTableListCache(); + } + + // Remove tables that no longer exist + allowedTables.keySet().forEach(tid -> { + if (!getContext().getTableIdToNameMap().containsKey(tid)) { + LOG.debug("Removing table {} from allowed table map as it no longer exists", tid); + allowedTables.remove(tid); + } + }); + + final String propName = Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + groupName; + String allowedTableRegex = getConfiguration().get(propName); + if (allowedTableRegex == null) { + allowedTableRegex = DEFAULT_SCAN_ALLOWED_PATTERN; + } + + if (currentAllowedTableRegex == null) { + LOG.debug("Property {} initial value: {}", propName, allowedTableRegex); + } else if (currentAllowedTableRegex.equals(allowedTableRegex)) { + // Property value has not changed, do nothing + } else { + LOG.debug("Property {} has changed. Old value: {}, new value: {}", propName, + currentAllowedTableRegex, allowedTableRegex); + } + + Pattern allowedTablePattern; + try { + allowedTablePattern = Pattern.compile(allowedTableRegex); + } catch (PatternSyntaxException e) { + LOG.error( + "Property {} contains an invalid regular expression. Property value: {}. Using default pattern: {}", + propName, allowedTableRegex, DEFAULT_SCAN_ALLOWED_PATTERN); + allowedTablePattern = Pattern.compile(DEFAULT_SCAN_ALLOWED_PATTERN); + } + // Regex is valid, store it + currentAllowedTableRegex = allowedTableRegex; + + Pattern p = allowedTablePattern; + context.getTableNameToIdMap().entrySet().forEach(e -> { + String tname = e.getKey(); + TableId tid = e.getValue(); + Matcher m = p.matcher(tname); + if (m.matches()) { + LOG.debug("Table {} can now be scanned via this ScanServer", tname); + allowedTables.put(tid, tid); + } else if (allowedTables.containsKey(tid)) { + LOG.debug("Table {} can no longer be scanned via this ScanServer", tname); + allowedTables.remove(tid); + } else { + LOG.debug("Table name: {} does not match regex: {}", tname, p.pattern()); + } + }); + + } + @SuppressWarnings("unchecked") private Map getTabletMetadata(Collection extents) { if (tabletMetadataCache == null) { @@ -945,11 +1029,6 @@ public void close() { }; } - /* Exposed for testing */ - protected boolean isSystemUser(TCredentials creds) { - return context.getSecurityOperation().isSystemUser(creds); - } - @Override public InitialScan startScan(TInfo tinfo, TCredentials credentials, TKeyExtent textent, TRange range, List columns, int batchSize, List ssiList, @@ -966,9 +1045,9 @@ public InitialScan startScan(TInfo tinfo, TCredentials credentials, TKeyExtent t KeyExtent extent = getKeyExtent(textent); - if (extent.isMeta() && !isSystemUser(credentials)) { - throw new TException( - "Only the system user can perform eventual consistency scans on the root and metadata tables"); + if (!isAllowed(extent.tableId())) { + throw new TException("Scan of table " + extent.tableId() + " disallowed by property: " + + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); } try (ScanReservation reservation = @@ -1038,9 +1117,9 @@ public InitialMultiScan startMultiScan(TInfo tinfo, TCredentials credentials, for (Entry> entry : tbatch.entrySet()) { KeyExtent extent = getKeyExtent(entry.getKey()); - if (extent.isMeta() && !context.getSecurityOperation().isSystemUser(credentials)) { - throw new TException( - "Only the system user can perform eventual consistency scans on the root and metadata tables"); + if (!isAllowed(extent.tableId())) { + throw new TException("Scan of table " + extent.tableId() + " disallowed by property: " + + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); } batch.put(extent, entry.getValue()); diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java b/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java index 5a080408720..3080004519c 100644 --- a/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java +++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java @@ -24,7 +24,9 @@ import static org.easymock.EasyMock.replay; import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.ByteBuffer; import java.util.ArrayList; @@ -32,8 +34,11 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.regex.Pattern; import org.apache.accumulo.core.client.AccumuloException; +import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.dataImpl.KeyExtent; import org.apache.accumulo.core.dataImpl.thrift.InitialMultiScan; import org.apache.accumulo.core.dataImpl.thrift.InitialScan; @@ -43,6 +48,7 @@ import org.apache.accumulo.core.dataImpl.thrift.TColumn; import org.apache.accumulo.core.dataImpl.thrift.TKeyExtent; import org.apache.accumulo.core.dataImpl.thrift.TRange; +import org.apache.accumulo.core.metadata.MetadataTable; import org.apache.accumulo.core.process.thrift.ServerProcessService; import org.apache.accumulo.core.securityImpl.thrift.TCredentials; import org.apache.accumulo.core.tabletserver.thrift.NoSuchScanIDException; @@ -65,7 +71,7 @@ public class TestScanServer extends ScanServer implements ServerProcessService.I private KeyExtent extent; private TabletResolver resolver; private ScanReservation reservation; - private boolean systemUser; + private ConcurrentHashMap allowedTables; protected TestScanServer(ScanServerOpts opts, String[] args) { super(opts, args); @@ -114,13 +120,17 @@ ScanReservation reserveFilesInstrumented(long scanId) { } @Override - protected boolean isSystemUser(TCredentials creds) { - return systemUser; + public boolean isShutdownRequested() { + return false; } @Override - public boolean isShutdownRequested() { - return false; + protected boolean isAllowed(TableId tid) { + return allowedTables.containsKey(tid); + } + + public void addAllowedTable(TableId tid) { + allowedTables.put(tid, tid); } } @@ -147,6 +157,8 @@ public void testScan() throws Exception { TabletResolver resolver = createMock(TabletResolver.class); TestScanServer ss = partialMockBuilder(TestScanServer.class).createMock(); + TableId tid = TableId.of("42"); + expect(sextent.tableId()).andReturn(tid).once(); expect(reservation.newTablet(ss, sextent)).andReturn(tablet); expect(reservation.getFailures()).andReturn(Map.of()).anyTimes(); reservation.close(); @@ -157,14 +169,15 @@ public void testScan() throws Exception { expect(handler.continueScan(tinfo, 15, 0L)).andReturn(new ScanResult()); handler.closeScan(tinfo, 15); - replay(reservation, handler); + replay(reservation, sextent, handler); + ss.allowedTables = new ConcurrentHashMap<>(); + ss.addAllowedTable(tid); ss.delegate = handler; ss.extent = sextent; ss.resolver = resolver; ss.reservation = reservation; ss.clientAddress = HostAndPort.fromParts("127.0.0.1", 1234); - ss.systemUser = false; TKeyExtent textent = createMock(TKeyExtent.class); InitialScan is = ss.startScan(tinfo, tcreds, textent, trange, tcols, 10, titer, ssio, auths, @@ -172,7 +185,7 @@ public void testScan() throws Exception { assertEquals(15, is.getScanID()); ss.continueScan(tinfo, is.getScanID(), 0L); ss.closeScan(tinfo, is.getScanID()); - verify(reservation, handler); + verify(reservation, sextent, handler); } @Test @@ -194,18 +207,20 @@ public void testScanTabletLoadFailure() throws Exception { Map execHints = new HashMap<>(); ScanReservation reservation = createMock(ScanReservation.class); - expect(extent.isMeta()).andReturn(false).anyTimes(); + TestScanServer ss = partialMockBuilder(TestScanServer.class).createMock(); + TableId tid = TableId.of("42"); + expect(extent.tableId()).andReturn(tid).once(); expect(extent.toThrift()).andReturn(textent).anyTimes(); expect(reservation.getFailures()).andReturn(Map.of(textent, ranges)); reservation.close(); replay(extent, reservation); - TestScanServer ss = partialMockBuilder(TestScanServer.class).createMock(); + ss.allowedTables = new ConcurrentHashMap<>(); + ss.addAllowedTable(tid); ss.extent = extent; ss.delegate = handler; ss.reservation = reservation; - ss.systemUser = false; assertThrows(NotServingTabletException.class, () -> { ss.startScan(tinfo, tcreds, textent, trange, tcols, 10, titer, ssio, auths, false, false, 10, @@ -246,7 +261,8 @@ public void close() {} }; TestScanServer ss = partialMockBuilder(TestScanServer.class).createMock(); - expect(extent.isMeta()).andReturn(false).anyTimes(); + TableId tid = TableId.of("42"); + expect(extent.tableId()).andReturn(tid).once(); expect(reservation.newTablet(ss, extent)).andReturn(tablet); expect(reservation.getTabletMetadataExtents()).andReturn(Set.of(extent)); expect(reservation.getFailures()).andReturn(Map.of()); @@ -259,12 +275,13 @@ public void close() {} replay(extent, reservation, handler); + ss.allowedTables = new ConcurrentHashMap<>(); + ss.addAllowedTable(tid); ss.delegate = handler; ss.extent = extent; ss.resolver = resolver; ss.reservation = reservation; ss.clientAddress = HostAndPort.fromParts("127.0.0.1", 1234); - ss.systemUser = false; Map> extents = new HashMap<>(); extents.put(createMock(TKeyExtent.class), ranges); @@ -309,7 +326,8 @@ public void close() {} }; TestScanServer ss = partialMockBuilder(TestScanServer.class).createMock(); - expect(extent.isMeta()).andReturn(false).anyTimes(); + TableId tid = TableId.of("42"); + expect(extent.tableId()).andReturn(tid).once(); expect(reservation.newTablet(ss, extent)).andReturn(tablet).anyTimes(); expect(reservation.getTabletMetadataExtents()).andReturn(Set.of()); expect(reservation.getFailures()).andReturn(Map.of(textent, ranges)).anyTimes(); @@ -321,12 +339,13 @@ public void close() {} replay(extent, reservation, handler); + ss.allowedTables = new ConcurrentHashMap<>(); + ss.addAllowedTable(tid); ss.delegate = handler; ss.extent = extent; ss.resolver = resolver; ss.reservation = reservation; ss.clientAddress = HostAndPort.fromParts("127.0.0.1", 1234); - ss.systemUser = false; Map> extents = new HashMap<>(); extents.put(textent, ranges); @@ -370,7 +389,6 @@ public void close() {} ss.delegate = handler; ss.resolver = resolver; ss.clientAddress = HostAndPort.fromParts("127.0.0.1", 1234); - ss.systemUser = false; assertThrows(TException.class, () -> { ss.startMultiScan(tinfo, tcreds, extents, tcols, titer, ssio, auths, false, tsc, 30L, @@ -380,7 +398,7 @@ public void close() {} } @Test - public void testScanMetaTablesSystemUser() throws Exception { + public void testScanDefaultAllowedTables() throws Exception { handler = createMock(ThriftScanClientHandler.class); TInfo tinfo = createMock(TInfo.class); @@ -399,7 +417,7 @@ public void testScanMetaTablesSystemUser() throws Exception { TabletResolver resolver = createMock(TabletResolver.class); TestScanServer ss = partialMockBuilder(TestScanServer.class).createMock(); - expect(sextent.isMeta()).andReturn(true).anyTimes(); + expect(sextent.tableId()).andReturn(MetadataTable.ID).once(); expect(reservation.newTablet(ss, sextent)).andReturn(tablet); expect(reservation.getFailures()).andReturn(Map.of()).anyTimes(); reservation.close(); @@ -412,12 +430,13 @@ public void testScanMetaTablesSystemUser() throws Exception { replay(sextent, reservation, handler); + ss.allowedTables = new ConcurrentHashMap<>(); + ss.addAllowedTable(MetadataTable.ID); ss.delegate = handler; ss.extent = sextent; ss.resolver = resolver; ss.reservation = reservation; ss.clientAddress = HostAndPort.fromParts("127.0.0.1", 1234); - ss.systemUser = true; TKeyExtent textent = createMock(TKeyExtent.class); InitialScan is = ss.startScan(tinfo, tcreds, textent, trange, tcols, 10, titer, ssio, auths, @@ -430,7 +449,7 @@ public void testScanMetaTablesSystemUser() throws Exception { } @Test - public void testScanMetaTablesNonSystemUser() throws Exception { + public void testScanDisallowedTable() throws Exception { handler = createMock(ThriftScanClientHandler.class); TInfo tinfo = createMock(TInfo.class); @@ -448,25 +467,37 @@ public void testScanMetaTablesNonSystemUser() throws Exception { TabletResolver resolver = createMock(TabletResolver.class); TestScanServer ss = partialMockBuilder(TestScanServer.class).createMock(); - expect(sextent.isMeta()).andReturn(true).anyTimes(); + expect(sextent.tableId()).andReturn(MetadataTable.ID).anyTimes(); expect(reservation.getFailures()).andReturn(Map.of()).anyTimes(); replay(sextent, reservation, handler); + ss.allowedTables = new ConcurrentHashMap<>(); + ss.addAllowedTable(TableId.of("42")); ss.delegate = handler; ss.extent = sextent; ss.resolver = resolver; ss.reservation = reservation; ss.clientAddress = HostAndPort.fromParts("127.0.0.1", 1234); - ss.systemUser = false; TKeyExtent textent = createMock(TKeyExtent.class); - assertThrows(TException.class, () -> { + TException te = assertThrows(TException.class, () -> { ss.startScan(tinfo, tcreds, textent, trange, tcols, 10, titer, ssio, auths, false, false, 10, tsc, 30L, classLoaderContext, execHints, 0L); }); + assertTrue(te.getMessage().startsWith("Scan of table !0 disallowed by property")); verify(sextent, reservation, handler); } + @Test + public void testTableNameRegex() { + String r = "^(?!accumulo\\.).*$"; + Pattern p = Pattern.compile(r); + + assertFalse(p.matcher("accumulo.root").matches()); + assertFalse(p.matcher("accumulo.metadata").matches()); + assertTrue(p.matcher("test.table").matches()); + } + } diff --git a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java new file mode 100644 index 00000000000..041da82b042 --- /dev/null +++ b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java @@ -0,0 +1,238 @@ +/* + * 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.test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Map; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.client.Accumulo; +import org.apache.accumulo.core.client.AccumuloClient; +import org.apache.accumulo.core.client.Scanner; +import org.apache.accumulo.core.client.ScannerBase.ConsistencyLevel; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.conf.ClientProperty; +import org.apache.accumulo.core.conf.Property; +import org.apache.accumulo.core.data.Range; +import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter; +import org.apache.accumulo.core.metadata.MetadataTable; +import org.apache.accumulo.core.security.Authorizations; +import org.apache.accumulo.core.spi.scan.ScanServerSelector; +import org.apache.accumulo.harness.MiniClusterConfigurationCallback; +import org.apache.accumulo.harness.SharedMiniClusterBase; +import org.apache.accumulo.minicluster.ServerType; +import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl; +import org.apache.accumulo.test.util.Wait; +import org.apache.accumulo.tserver.ScanServer; +import org.apache.hadoop.conf.Configuration; +import org.apache.zookeeper.ZooKeeper; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import com.google.common.collect.Iterables; + +public class ScanServerAllowedTablesIT extends SharedMiniClusterBase { + + // @formatter:off + private static final String clientConfiguration = + "["+ + " {"+ + " \"isDefault\": true,"+ + " \"maxBusyTimeout\": \"5m\","+ + " \"busyTimeoutMultiplier\": 8,"+ + " \"scanTypeActivations\": [],"+ + " \"attemptPlans\": ["+ + " {"+ + " \"servers\": \"3\","+ + " \"busyTimeout\": \"33ms\","+ + " \"salt\": \"one\""+ + " },"+ + " {"+ + " \"servers\": \"13\","+ + " \"busyTimeout\": \"33ms\","+ + " \"salt\": \"two\""+ + " },"+ + " {"+ + " \"servers\": \"100%\","+ + " \"busyTimeout\": \"33ms\""+ + " }"+ + " ]"+ + " },"+ + " {"+ + " \"isDefault\": false,"+ + " \"maxBusyTimeout\": \"5m\","+ + " \"busyTimeoutMultiplier\": 8,"+ + " \"group\": \"GROUP1\","+ + " \"scanTypeActivations\": [\"use_group1\"],"+ + " \"attemptPlans\": ["+ + " {"+ + " \"servers\": \"3\","+ + " \"busyTimeout\": \"33ms\","+ + " \"salt\": \"one\""+ + " },"+ + " {"+ + " \"servers\": \"13\","+ + " \"busyTimeout\": \"33ms\","+ + " \"salt\": \"two\""+ + " },"+ + " {"+ + " \"servers\": \"100%\","+ + " \"busyTimeout\": \"33ms\""+ + " }"+ + " ]"+ + " }"+ + "]"; + // @formatter:on + + public static class SSATITConfiguration implements MiniClusterConfigurationCallback { + + @Override + public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration coreSite) { + + cfg.setNumScanServers(1); + + // allow the ScanServer in the DEFAULT group to only scan tables in accumulo namespace + cfg.setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + + ScanServerSelector.DEFAULT_SCAN_SERVER_GROUP_NAME, "^accumulo\\..*$"); + // allow the ScanServer in the GROUP1 group to only scan tables created with the prefix 'test' + cfg.setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + "GROUP1", "^test.*"); + + cfg.setClientProperty(ClientProperty.SCAN_SERVER_SELECTOR_OPTS_PREFIX.getKey() + "profiles", + clientConfiguration); + } + + } + + @BeforeAll + public static void start() throws Exception { + SharedMiniClusterBase.startMiniClusterWithConfig(new SSATITConfiguration()); + SharedMiniClusterBase.getCluster().getClusterControl().start(ServerType.SCAN_SERVER, + "localhost"); + + String zooRoot = getCluster().getServerContext().getZooKeeperRoot(); + ZooReaderWriter zrw = getCluster().getServerContext().getZooReaderWriter(); + String scanServerRoot = zooRoot + Constants.ZSSERVERS; + + while (zrw.getChildren(scanServerRoot).size() == 0) { + Thread.sleep(500); + } + } + + @AfterAll + public static void stop() throws Exception { + SharedMiniClusterBase.stopMiniCluster(); + } + + @Test + public void testAllowedTables() throws Exception { + + final String zooRoot = getCluster().getServerContext().getZooKeeperRoot(); + final ZooKeeper zk = getCluster().getServerContext().getZooReaderWriter().getZooKeeper(); + final String scanServerRoot = zooRoot + Constants.ZSSERVERS; + + try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) { + + // Start the 2nd ScanServer + // Bump the number of scan serves that can run to start the GROUP1 scan server + getCluster().getConfig().setNumScanServers(2); + getCluster()._exec(ScanServer.class, ServerType.SCAN_SERVER, Map.of(), + new String[] {"-g", "GROUP1"}); + Wait.waitFor(() -> zk.getChildren(scanServerRoot, false).size() == 2); + Wait.waitFor(() -> ((ClientContext) client).getScanServers().values().stream().anyMatch( + (p) -> p.getSecond().equals(ScanServerSelector.DEFAULT_SCAN_SERVER_GROUP_NAME)) == true); + Wait.waitFor(() -> ((ClientContext) client).getScanServers().values().stream() + .anyMatch((p) -> p.getSecond().equals("GROUP1")) == true); + + // Create table with test prefix, load some data + final String testTableName = "testAllowedTables"; + final int ingestedEntryCount = + ScanServerIT.createTableAndIngest(client, testTableName, null, 10, 10, "colf"); + assertEquals(100, ingestedEntryCount); + + // Using default ScanServer should succeed, only allowed to scan system tables + try (Scanner scanner = client.createScanner(MetadataTable.NAME, Authorizations.EMPTY)) { + scanner.setRange(new Range()); + scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); + assertTrue(Iterables.size(scanner) > 0); + } + + // Using default ScanServer should fail, only allowed to scan system tables + try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { + scanner.setRange(new Range()); + scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); + assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + } + + // Using GROUP1 ScanServer should fail, only allowed to test tables + try (Scanner scanner = client.createScanner(MetadataTable.NAME, Authorizations.EMPTY)) { + scanner.setRange(new Range()); + scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); + scanner.setExecutionHints(Map.of("scan_type", "use_group1")); + assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + } + + // Using GROUP1 ScanServer should succeed + try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { + scanner.setRange(new Range()); + scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); + scanner.setExecutionHints(Map.of("scan_type", "use_group1")); + assertEquals(100, Iterables.size(scanner)); + } + + // Change the GROUP1 property so that subsequent test tables don't work + getCluster().getServerContext().instanceOperations() + .setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + "GROUP1", "^foo.*"); + + // Using GROUP1 ScanServer should fail, only allowed to test tables + try (Scanner scanner = client.createScanner(MetadataTable.NAME, Authorizations.EMPTY)) { + scanner.setRange(new Range()); + scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); + scanner.setExecutionHints(Map.of("scan_type", "use_group1")); + assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + } + + // Using GROUP1 ScanServer should fail as the property was changed + try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { + scanner.setRange(new Range()); + scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); + scanner.setExecutionHints(Map.of("scan_type", "use_group1")); + assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + } + + // Change the GROUP1 property so that subsequent test tables don't work + getCluster().getServerContext().instanceOperations() + .setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + "GROUP1", "^test.*"); + + // Using GROUP1 ScanServer should succeed as the property was changed back + try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { + scanner.setRange(new Range()); + scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); + scanner.setExecutionHints(Map.of("scan_type", "use_group1")); + assertEquals(100, Iterables.size(scanner)); + } + + } + + } + +} From 881b810e137044ce83c7291dfc36663d0ef46580 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Mon, 23 Feb 2026 20:38:52 +0000 Subject: [PATCH 2/6] Changed allowed table map from TableId to Boolean --- .../org/apache/accumulo/tserver/ScanServer.java | 16 +++++++--------- .../accumulo/test/ScanServerAllowedTablesIT.java | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java index 0d14f5bd3ae..780a6fa52f6 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java @@ -219,7 +219,7 @@ private TabletMetadataLoader(Ample ample) { private final String groupName; - private final ConcurrentHashMap allowedTables = new ConcurrentHashMap<>(); + private final ConcurrentHashMap allowedTables = new ConcurrentHashMap<>(); private volatile String currentAllowedTableRegex; public ScanServer(ScanServerOpts opts, String[] args) { @@ -490,13 +490,13 @@ public void run() { // Visible for testing protected boolean isAllowed(TableId tid) { - boolean result = allowedTables.containsKey(tid); - if (!result) { + Boolean result = allowedTables.get(tid); + if (result == null) { // Clear the cache and try again, maybe there // is a race condition in table creation and // scan updateAllowedTables(true); - result = allowedTables.containsKey(tid); + result = allowedTables.get(tid); } return result; } @@ -550,12 +550,10 @@ private synchronized void updateAllowedTables(boolean clearCache) { Matcher m = p.matcher(tname); if (m.matches()) { LOG.debug("Table {} can now be scanned via this ScanServer", tname); - allowedTables.put(tid, tid); - } else if (allowedTables.containsKey(tid)) { - LOG.debug("Table {} can no longer be scanned via this ScanServer", tname); - allowedTables.remove(tid); + allowedTables.put(tid, Boolean.TRUE); } else { - LOG.debug("Table name: {} does not match regex: {}", tname, p.pattern()); + LOG.debug("Table {} cannot be scanned via this ScanServer", tname); + allowedTables.put(tid, Boolean.FALSE); } }); diff --git a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java index 041da82b042..55223add34a 100644 --- a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java +++ b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java @@ -216,7 +216,7 @@ public void testAllowedTables() throws Exception { scanner.setRange(new Range()); scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); - assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + Wait.waitFor(() -> Iterables.size(scanner) == 100); } // Change the GROUP1 property so that subsequent test tables don't work From f3ca1307166a89fb964fa08de36d9802c62eadbd Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Tue, 24 Feb 2026 15:27:59 +0000 Subject: [PATCH 3/6] Protect against NPE from deleted table --- .../org/apache/accumulo/tserver/ScanServer.java | 17 +++++++++++------ .../tserver/ThriftScanClientHandler.java | 2 +- .../apache/accumulo/tserver/ScanServerTest.java | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java index 780a6fa52f6..7c1d06ca0f1 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java @@ -489,13 +489,18 @@ public void run() { } // Visible for testing - protected boolean isAllowed(TableId tid) { + protected boolean isAllowed(TCredentials credentials, TableId tid) + throws ThriftSecurityException { Boolean result = allowedTables.get(tid); - if (result == null) { + while (result == null) { + LOG.debug( + "Allowed tables mapping does not contain an entry for table: {}, refreshing table...", + tid); // Clear the cache and try again, maybe there - // is a race condition in table creation and - // scan + // is a race condition in table creation and scan updateAllowedTables(true); + // validate that the table exists, else throw + delegate.getNamespaceId(credentials, tid); result = allowedTables.get(tid); } return result; @@ -1043,7 +1048,7 @@ public InitialScan startScan(TInfo tinfo, TCredentials credentials, TKeyExtent t KeyExtent extent = getKeyExtent(textent); - if (!isAllowed(extent.tableId())) { + if (!isAllowed(credentials, extent.tableId())) { throw new TException("Scan of table " + extent.tableId() + " disallowed by property: " + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); } @@ -1115,7 +1120,7 @@ public InitialMultiScan startMultiScan(TInfo tinfo, TCredentials credentials, for (Entry> entry : tbatch.entrySet()) { KeyExtent extent = getKeyExtent(entry.getKey()); - if (!isAllowed(extent.tableId())) { + if (!isAllowed(credentials, extent.tableId())) { throw new TException("Scan of table " + extent.tableId() + " disallowed by property: " + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); } diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java index b254fd3e405..e0a54c83745 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java @@ -109,7 +109,7 @@ public ThriftScanClientHandler(TabletHostingServer server, WriteTracker writeTra .getTimeInMillis(Property.TSERV_SCAN_RESULTS_MAX_TIMEOUT); } - private NamespaceId getNamespaceId(TCredentials credentials, TableId tableId) + public NamespaceId getNamespaceId(TCredentials credentials, TableId tableId) throws ThriftSecurityException { try { return server.getContext().getNamespaceId(tableId); diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java b/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java index 3080004519c..fd4b19da009 100644 --- a/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java +++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java @@ -125,7 +125,7 @@ public boolean isShutdownRequested() { } @Override - protected boolean isAllowed(TableId tid) { + protected boolean isAllowed(TCredentials credentials, TableId tid) { return allowedTables.containsKey(tid); } From 02b55d1681beb6978d8bcf5e9a9f103ff011b222 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Tue, 24 Feb 2026 16:13:51 +0000 Subject: [PATCH 4/6] Updates from PR comments --- .../apache/accumulo/tserver/ScanServer.java | 33 +++++++++++-------- .../test/ScanServerAllowedTablesIT.java | 25 +++++++++++--- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java index 7c1d06ca0f1..374298b6075 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java @@ -53,6 +53,7 @@ import org.apache.accumulo.core.Constants; import org.apache.accumulo.core.client.AccumuloException; +import org.apache.accumulo.core.clientImpl.thrift.SecurityErrorCode; import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException; import org.apache.accumulo.core.conf.AccumuloConfiguration; import org.apache.accumulo.core.conf.Property; @@ -539,26 +540,30 @@ private synchronized void updateAllowedTables(boolean clearCache) { Pattern allowedTablePattern; try { allowedTablePattern = Pattern.compile(allowedTableRegex); + // Regex is valid, store it + currentAllowedTableRegex = allowedTableRegex; } catch (PatternSyntaxException e) { LOG.error( - "Property {} contains an invalid regular expression. Property value: {}. Using default pattern: {}", - propName, allowedTableRegex, DEFAULT_SCAN_ALLOWED_PATTERN); - allowedTablePattern = Pattern.compile(DEFAULT_SCAN_ALLOWED_PATTERN); + "Property {} contains an invalid regular expression. Property value: {}. Disabling all tables.", + propName, allowedTableRegex); + allowedTablePattern = null; } - // Regex is valid, store it - currentAllowedTableRegex = allowedTableRegex; Pattern p = allowedTablePattern; context.getTableNameToIdMap().entrySet().forEach(e -> { String tname = e.getKey(); TableId tid = e.getValue(); - Matcher m = p.matcher(tname); - if (m.matches()) { - LOG.debug("Table {} can now be scanned via this ScanServer", tname); - allowedTables.put(tid, Boolean.TRUE); - } else { - LOG.debug("Table {} cannot be scanned via this ScanServer", tname); + if (p == null) { allowedTables.put(tid, Boolean.FALSE); + } else { + Matcher m = p.matcher(tname); + if (m.matches()) { + LOG.debug("Table {} can now be scanned via this ScanServer", tname); + allowedTables.put(tid, Boolean.TRUE); + } else { + LOG.debug("Table {} cannot be scanned via this ScanServer", tname); + allowedTables.put(tid, Boolean.FALSE); + } } }); @@ -1121,8 +1126,10 @@ public InitialMultiScan startMultiScan(TInfo tinfo, TCredentials credentials, KeyExtent extent = getKeyExtent(entry.getKey()); if (!isAllowed(credentials, extent.tableId())) { - throw new TException("Scan of table " + extent.tableId() + " disallowed by property: " - + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); + throw new ThriftSecurityException( + "Scan of table " + extent.tableId() + " disallowed by property: " + + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName, + SecurityErrorCode.PERMISSION_DENIED); } batch.put(extent, entry.getValue()); diff --git a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java index 55223add34a..c3463b14f9a 100644 --- a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java +++ b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java @@ -203,7 +203,7 @@ public void testAllowedTables() throws Exception { getCluster().getServerContext().instanceOperations() .setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + "GROUP1", "^foo.*"); - // Using GROUP1 ScanServer should fail, only allowed to test tables + // Using GROUP1 ScanServer should fail, only allowed to test 'test*' tables try (Scanner scanner = client.createScanner(MetadataTable.NAME, Authorizations.EMPTY)) { scanner.setRange(new Range()); scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); @@ -216,10 +216,18 @@ public void testAllowedTables() throws Exception { scanner.setRange(new Range()); scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); - Wait.waitFor(() -> Iterables.size(scanner) == 100); + // Try multiple times waiting for the server to pick up the property change + Wait.waitFor(() -> { + try { + Iterables.size(scanner); + return false; + } catch (RuntimeException e) { + return true; + } + }); } - // Change the GROUP1 property so that subsequent test tables don't work + // Change the GROUP1 property so that subsequent test tables do work getCluster().getServerContext().instanceOperations() .setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + "GROUP1", "^test.*"); @@ -228,7 +236,16 @@ public void testAllowedTables() throws Exception { scanner.setRange(new Range()); scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); - assertEquals(100, Iterables.size(scanner)); + // Try multiple times waiting for the server to pick up the property change + Wait.waitFor(() -> { + try { + int size = Iterables.size(scanner); + return size == 100; + } catch (RuntimeException e) { + return false; + } + }); + } } From 9ed878f2f36d920d069aeee0336ce3b65ff7b497 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Tue, 24 Feb 2026 16:32:24 +0000 Subject: [PATCH 5/6] Fix errorprone issue in ScanServerAllowedTablesIT --- .../org/apache/accumulo/test/ScanServerAllowedTablesIT.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java index c3463b14f9a..baa9d346f67 100644 --- a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java +++ b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java @@ -143,6 +143,7 @@ public static void stop() throws Exception { SharedMiniClusterBase.stopMiniCluster(); } + @SuppressWarnings("unused") @Test public void testAllowedTables() throws Exception { @@ -219,7 +220,7 @@ public void testAllowedTables() throws Exception { // Try multiple times waiting for the server to pick up the property change Wait.waitFor(() -> { try { - Iterables.size(scanner); + var unused = Iterables.size(scanner); return false; } catch (RuntimeException e) { return true; From 3ea6c9ffed562dae08c65805a4ec1912d9e3a37f Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Tue, 24 Feb 2026 22:59:43 +0000 Subject: [PATCH 6/6] Implemented more PR suggestions --- .../apache/accumulo/tserver/ScanServer.java | 66 ++++++++++------ .../accumulo/tserver/ScanServerTest.java | 6 +- .../test/ScanServerAllowedTablesIT.java | 78 ++++++++++++++----- 3 files changed, 106 insertions(+), 44 deletions(-) diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java index 374298b6075..53b121f08d5 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java @@ -19,6 +19,7 @@ package org.apache.accumulo.tserver; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.apache.accumulo.core.util.UtilWaitThread.sleepUninterruptibly; import static org.apache.accumulo.core.util.threads.ThreadPoolNames.SCAN_SERVER_TABLET_METADATA_CACHE_POOL; @@ -53,7 +54,6 @@ import org.apache.accumulo.core.Constants; import org.apache.accumulo.core.client.AccumuloException; -import org.apache.accumulo.core.clientImpl.thrift.SecurityErrorCode; import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException; import org.apache.accumulo.core.conf.AccumuloConfiguration; import org.apache.accumulo.core.conf.Property; @@ -93,6 +93,7 @@ import org.apache.accumulo.core.tabletserver.thrift.TooManyFilesException; import org.apache.accumulo.core.trace.thrift.TInfo; import org.apache.accumulo.core.util.HostAndPort; +import org.apache.accumulo.core.util.Retry; import org.apache.accumulo.core.util.UtilWaitThread; import org.apache.accumulo.core.util.threads.ThreadPools; import org.apache.accumulo.server.AbstractServer; @@ -116,6 +117,7 @@ import org.apache.accumulo.tserver.tablet.SnapshotTablet; import org.apache.accumulo.tserver.tablet.Tablet; import org.apache.accumulo.tserver.tablet.TabletBase; +import org.apache.thrift.TApplicationException; import org.apache.thrift.TException; import org.apache.thrift.TProcessor; import org.apache.zookeeper.KeeperException; @@ -493,23 +495,43 @@ public void run() { protected boolean isAllowed(TCredentials credentials, TableId tid) throws ThriftSecurityException { Boolean result = allowedTables.get(tid); - while (result == null) { - LOG.debug( - "Allowed tables mapping does not contain an entry for table: {}, refreshing table...", - tid); - // Clear the cache and try again, maybe there - // is a race condition in table creation and scan - updateAllowedTables(true); - // validate that the table exists, else throw - delegate.getNamespaceId(credentials, tid); - result = allowedTables.get(tid); + if (result == null) { + + final Retry retry = + Retry.builder().maxRetries(10).retryAfter(1, SECONDS).incrementBy(0, SECONDS) + .maxWait(2, SECONDS).backOffFactor(1.0).logInterval(3, SECONDS).createRetry(); + + while (result == null && retry.canRetry()) { + try { + retry.waitForNextAttempt(LOG, + "Allowed tables mapping does not contain an entry for table: " + tid + + ", refreshing table..."); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + LOG.error("Interrupted while waiting for next retry", e); + break; + } + // Clear the cache and try again, maybe there + // is a race condition in table creation and scan + updateAllowedTables(true); + // validate that the table exists, else throw + delegate.getNamespaceId(credentials, tid); + result = allowedTables.get(tid); + retry.useRetry(); + } + + if (result == null) { + // Ran out of retries + throw new IllegalStateException( + "Unable to get allowed table mapping for table: " + tid + " within 10s"); + } } return result; } private synchronized void updateAllowedTables(boolean clearCache) { - LOG.debug("Updating allowed tables for ScanServer"); + LOG.trace("Updating allowed tables for ScanServer"); if (clearCache) { context.clearTableListCache(); } @@ -517,7 +539,7 @@ private synchronized void updateAllowedTables(boolean clearCache) { // Remove tables that no longer exist allowedTables.keySet().forEach(tid -> { if (!getContext().getTableIdToNameMap().containsKey(tid)) { - LOG.debug("Removing table {} from allowed table map as it no longer exists", tid); + LOG.trace("Removing table {} from allowed table map as it no longer exists", tid); allowedTables.remove(tid); } }); @@ -529,11 +551,11 @@ private synchronized void updateAllowedTables(boolean clearCache) { } if (currentAllowedTableRegex == null) { - LOG.debug("Property {} initial value: {}", propName, allowedTableRegex); + LOG.trace("Property {} initial value: {}", propName, allowedTableRegex); } else if (currentAllowedTableRegex.equals(allowedTableRegex)) { // Property value has not changed, do nothing } else { - LOG.debug("Property {} has changed. Old value: {}, new value: {}", propName, + LOG.info("Property {} has changed. Old value: {}, new value: {}", propName, currentAllowedTableRegex, allowedTableRegex); } @@ -558,10 +580,10 @@ private synchronized void updateAllowedTables(boolean clearCache) { } else { Matcher m = p.matcher(tname); if (m.matches()) { - LOG.debug("Table {} can now be scanned via this ScanServer", tname); + LOG.trace("Table {} can now be scanned via this ScanServer", tname); allowedTables.put(tid, Boolean.TRUE); } else { - LOG.debug("Table {} cannot be scanned via this ScanServer", tname); + LOG.trace("Table {} cannot be scanned via this ScanServer", tname); allowedTables.put(tid, Boolean.FALSE); } } @@ -1054,8 +1076,9 @@ public InitialScan startScan(TInfo tinfo, TCredentials credentials, TKeyExtent t KeyExtent extent = getKeyExtent(textent); if (!isAllowed(credentials, extent.tableId())) { - throw new TException("Scan of table " + extent.tableId() + " disallowed by property: " - + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); + throw new TApplicationException(TApplicationException.INTERNAL_ERROR, + "Scan of table " + extent.tableId() + " disallowed by property: " + + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); } try (ScanReservation reservation = @@ -1126,10 +1149,9 @@ public InitialMultiScan startMultiScan(TInfo tinfo, TCredentials credentials, KeyExtent extent = getKeyExtent(entry.getKey()); if (!isAllowed(credentials, extent.tableId())) { - throw new ThriftSecurityException( + throw new TApplicationException(TApplicationException.INTERNAL_ERROR, "Scan of table " + extent.tableId() + " disallowed by property: " - + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName, - SecurityErrorCode.PERMISSION_DENIED); + + Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + this.groupName); } batch.put(extent, entry.getValue()); diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java b/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java index fd4b19da009..968f75e1b26 100644 --- a/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java +++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/ScanServerTest.java @@ -61,6 +61,7 @@ import org.apache.accumulo.tserver.tablet.SnapshotTablet; import org.apache.accumulo.tserver.tablet.Tablet; import org.apache.accumulo.tserver.tablet.TabletBase; +import org.apache.thrift.TApplicationException; import org.apache.thrift.TException; import org.junit.jupiter.api.Test; @@ -485,7 +486,10 @@ public void testScanDisallowedTable() throws Exception { ss.startScan(tinfo, tcreds, textent, trange, tcols, 10, titer, ssio, auths, false, false, 10, tsc, 30L, classLoaderContext, execHints, 0L); }); - assertTrue(te.getMessage().startsWith("Scan of table !0 disallowed by property")); + assertTrue(te instanceof TApplicationException); + TApplicationException tae = (TApplicationException) te; + assertEquals(TApplicationException.INTERNAL_ERROR, tae.getType()); + assertTrue(tae.getMessage().contains("disallowed by property")); verify(sextent, reservation, handler); } diff --git a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java index baa9d346f67..e14834cf483 100644 --- a/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java +++ b/test/src/main/java/org/apache/accumulo/test/ScanServerAllowedTablesIT.java @@ -23,12 +23,16 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Map; +import java.util.Set; import org.apache.accumulo.core.Constants; import org.apache.accumulo.core.client.Accumulo; import org.apache.accumulo.core.client.AccumuloClient; +import org.apache.accumulo.core.client.BatchScanner; import org.apache.accumulo.core.client.Scanner; +import org.apache.accumulo.core.client.ScannerBase; import org.apache.accumulo.core.client.ScannerBase.ConsistencyLevel; +import org.apache.accumulo.core.client.TableNotFoundException; import org.apache.accumulo.core.clientImpl.ClientContext; import org.apache.accumulo.core.conf.ClientProperty; import org.apache.accumulo.core.conf.Property; @@ -43,11 +47,14 @@ import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl; import org.apache.accumulo.test.util.Wait; import org.apache.accumulo.tserver.ScanServer; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.hadoop.conf.Configuration; +import org.apache.thrift.TApplicationException; import org.apache.zookeeper.ZooKeeper; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import com.google.common.collect.Iterables; @@ -143,9 +150,30 @@ public static void stop() throws Exception { SharedMiniClusterBase.stopMiniCluster(); } + public static enum ScannerType { + BATCH_SCANNER, SCANNER; + } + + private ScannerBase createScanner(AccumuloClient client, ScannerType stype, String tableName) + throws TableNotFoundException { + switch (stype) { + case BATCH_SCANNER: + BatchScanner batchScanner = client.createBatchScanner(tableName, Authorizations.EMPTY); + batchScanner.setRanges(Set.of(new Range())); + return batchScanner; + case SCANNER: + Scanner scanner = client.createScanner(tableName, Authorizations.EMPTY); + scanner.setRange(new Range()); + return scanner; + default: + throw new IllegalArgumentException("Unknown scanner type: " + stype); + } + } + @SuppressWarnings("unused") - @Test - public void testAllowedTables() throws Exception { + @ParameterizedTest + @EnumSource(value = ScannerType.class) + public void testAllowedTables(ScannerType stype) throws Exception { final String zooRoot = getCluster().getServerContext().getZooKeeperRoot(); final ZooKeeper zk = getCluster().getServerContext().getZooReaderWriter().getZooKeeper(); @@ -165,36 +193,42 @@ public void testAllowedTables() throws Exception { .anyMatch((p) -> p.getSecond().equals("GROUP1")) == true); // Create table with test prefix, load some data - final String testTableName = "testAllowedTables"; + final String testTableName = "testAllowedTables" + stype.name(); final int ingestedEntryCount = ScanServerIT.createTableAndIngest(client, testTableName, null, 10, 10, "colf"); assertEquals(100, ingestedEntryCount); // Using default ScanServer should succeed, only allowed to scan system tables - try (Scanner scanner = client.createScanner(MetadataTable.NAME, Authorizations.EMPTY)) { - scanner.setRange(new Range()); + try (ScannerBase scanner = createScanner(client, stype, MetadataTable.NAME)) { scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); assertTrue(Iterables.size(scanner) > 0); } // Using default ScanServer should fail, only allowed to scan system tables - try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { - scanner.setRange(new Range()); + try (ScannerBase scanner = createScanner(client, stype, testTableName)) { scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); - assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + RuntimeException re = assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + Throwable root = ExceptionUtils.getRootCause(re); + assertTrue(root instanceof TApplicationException); + TApplicationException tae = (TApplicationException) root; + assertEquals(TApplicationException.INTERNAL_ERROR, tae.getType()); + assertTrue(tae.getMessage().contains("disallowed by property")); } // Using GROUP1 ScanServer should fail, only allowed to test tables - try (Scanner scanner = client.createScanner(MetadataTable.NAME, Authorizations.EMPTY)) { - scanner.setRange(new Range()); + try (ScannerBase scanner = createScanner(client, stype, MetadataTable.NAME)) { scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); - assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + RuntimeException re = assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + Throwable root = ExceptionUtils.getRootCause(re); + assertTrue(root instanceof TApplicationException); + TApplicationException tae = (TApplicationException) root; + assertEquals(TApplicationException.INTERNAL_ERROR, tae.getType()); + assertTrue(tae.getMessage().contains("disallowed by property")); } // Using GROUP1 ScanServer should succeed - try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { - scanner.setRange(new Range()); + try (ScannerBase scanner = createScanner(client, stype, testTableName)) { scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); assertEquals(100, Iterables.size(scanner)); @@ -205,16 +239,19 @@ public void testAllowedTables() throws Exception { .setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + "GROUP1", "^foo.*"); // Using GROUP1 ScanServer should fail, only allowed to test 'test*' tables - try (Scanner scanner = client.createScanner(MetadataTable.NAME, Authorizations.EMPTY)) { - scanner.setRange(new Range()); + try (ScannerBase scanner = createScanner(client, stype, MetadataTable.NAME)) { scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); - assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + RuntimeException re = assertThrows(RuntimeException.class, () -> Iterables.size(scanner)); + Throwable root = ExceptionUtils.getRootCause(re); + assertTrue(root instanceof TApplicationException); + TApplicationException tae = (TApplicationException) root; + assertEquals(TApplicationException.INTERNAL_ERROR, tae.getType()); + assertTrue(tae.getMessage().contains("disallowed by property")); } // Using GROUP1 ScanServer should fail as the property was changed - try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { - scanner.setRange(new Range()); + try (ScannerBase scanner = createScanner(client, stype, testTableName)) { scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); // Try multiple times waiting for the server to pick up the property change @@ -233,8 +270,7 @@ public void testAllowedTables() throws Exception { .setProperty(Property.SSERV_SCAN_ALLOWED_TABLES.getKey() + "GROUP1", "^test.*"); // Using GROUP1 ScanServer should succeed as the property was changed back - try (Scanner scanner = client.createScanner(testTableName, Authorizations.EMPTY)) { - scanner.setRange(new Range()); + try (ScannerBase scanner = createScanner(client, stype, testTableName)) { scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); scanner.setExecutionHints(Map.of("scan_type", "use_group1")); // Try multiple times waiting for the server to pick up the property change