You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by ct...@apache.org on 2020/03/17 06:16:06 UTC
[accumulo] branch master updated: Fix #1430 Enforce table name
limit for new tables (#1518)
This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/master by this push:
new 88fa927 Fix #1430 Enforce table name limit for new tables (#1518)
88fa927 is described below
commit 88fa927a789f73ca063f63c95883273da60eddd4
Author: cradal <cd...@gmail.com>
AuthorDate: Tue Mar 17 02:15:57 2020 -0400
Fix #1430 Enforce table name limit for new tables (#1518)
* Add name length limit for new tables and namespaces (max length 1024 chars)
* Add fast failure for client side
* Warn for old names if beyond the max length
* Add tests
Co-Authored-By: Laura Schanno <lb...@gmail.com>
Co-Authored-By: Keith Turner <kt...@apache.org>
Co-Authored-By: Christopher Tubbs <ct...@apache.org>
---
.../java/org/apache/accumulo/core/Constants.java | 3 ++
.../core/clientImpl/NamespaceOperationsImpl.java | 8 +++-
.../core/clientImpl/TableOperationsImpl.java | 10 +++-
.../apache/accumulo/master/FateServiceHandler.java | 55 +++++++++++++++++++---
.../org/apache/accumulo/test/NamespacesIT.java | 17 +++++++
.../apache/accumulo/test/TableOperationsIT.java | 30 ++++++++----
6 files changed, 104 insertions(+), 19 deletions(-)
diff --git a/core/src/main/java/org/apache/accumulo/core/Constants.java b/core/src/main/java/org/apache/accumulo/core/Constants.java
index 85fa56a..eea31d8 100644
--- a/core/src/main/java/org/apache/accumulo/core/Constants.java
+++ b/core/src/main/java/org/apache/accumulo/core/Constants.java
@@ -114,4 +114,7 @@ public class Constants {
public static final String HDFS_TABLES_DIR = "/tables";
public static final int DEFAULT_VISIBILITY_CACHE_SIZE = 1000;
+
+ public static final int MAX_TABLE_NAME_LEN = 1024;
+ public static final int MAX_NAMESPACE_LEN = 1024;
}
diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java
index 7ad740b..e766c1e 100644
--- a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java
+++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java
@@ -20,6 +20,8 @@ package org.apache.accumulo.core.clientImpl;
import static com.google.common.base.Preconditions.checkArgument;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.accumulo.core.Constants.MAX_NAMESPACE_LEN;
+import static org.apache.accumulo.core.Constants.MAX_TABLE_NAME_LEN;
import java.nio.ByteBuffer;
import java.util.Arrays;
@@ -119,7 +121,8 @@ public class NamespaceOperationsImpl extends NamespaceOperationsHelper {
public void create(String namespace)
throws AccumuloException, AccumuloSecurityException, NamespaceExistsException {
checkArgument(namespace != null, "namespace is null");
-
+ checkArgument(namespace.length() <= MAX_NAMESPACE_LEN,
+ "Namespace is longer than " + MAX_NAMESPACE_LEN + " characters");
try {
doNamespaceFateOperation(FateOperation.NAMESPACE_CREATE,
Arrays.asList(ByteBuffer.wrap(namespace.getBytes(UTF_8))), Collections.emptyMap(),
@@ -163,7 +166,8 @@ public class NamespaceOperationsImpl extends NamespaceOperationsHelper {
public void rename(String oldNamespaceName, String newNamespaceName)
throws AccumuloSecurityException, NamespaceNotFoundException, AccumuloException,
NamespaceExistsException {
-
+ checkArgument(newNamespaceName.length() <= MAX_TABLE_NAME_LEN,
+ "Namespace is longer than " + MAX_TABLE_NAME_LEN + " characters");
List<ByteBuffer> args = Arrays.asList(ByteBuffer.wrap(oldNamespaceName.getBytes(UTF_8)),
ByteBuffer.wrap(newNamespaceName.getBytes(UTF_8)));
Map<String,String> opts = new HashMap<>();
diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
index 13cffb6..1e89e04 100644
--- a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
+++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
@@ -24,6 +24,7 @@ import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static java.util.stream.Collectors.toSet;
+import static org.apache.accumulo.core.Constants.MAX_TABLE_NAME_LEN;
import static org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType.LOCATION;
import static org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType.PREV_ROW;
import static org.apache.accumulo.fate.util.UtilWaitThread.sleepUninterruptibly;
@@ -217,6 +218,8 @@ public class TableOperationsImpl extends TableOperationsHelper {
throws AccumuloException, AccumuloSecurityException, TableExistsException {
checkArgument(tableName != null, "tableName is null");
checkArgument(ntc != null, "ntc is null");
+ checkArgument(tableName.length() <= MAX_TABLE_NAME_LEN,
+ "Table name is longer than " + MAX_TABLE_NAME_LEN + " characters");
List<ByteBuffer> args = new ArrayList<>();
args.add(ByteBuffer.wrap(tableName.getBytes(UTF_8)));
@@ -752,6 +755,8 @@ public class TableOperationsImpl extends TableOperationsHelper {
checkArgument(srcTableName != null, "srcTableName is null");
checkArgument(newTableName != null, "newTableName is null");
+ checkArgument(newTableName.length() <= MAX_TABLE_NAME_LEN,
+ "Table name is longer than " + MAX_TABLE_NAME_LEN + " characters");
TableId srcTableId = Tables.getTableId(context, srcTableName);
@@ -787,7 +792,8 @@ public class TableOperationsImpl extends TableOperationsHelper {
@Override
public void rename(String oldTableName, String newTableName) throws AccumuloSecurityException,
TableNotFoundException, AccumuloException, TableExistsException {
-
+ checkArgument(newTableName.length() <= MAX_TABLE_NAME_LEN,
+ "Table name is longer than " + MAX_TABLE_NAME_LEN + " characters");
List<ByteBuffer> args = Arrays.asList(ByteBuffer.wrap(oldTableName.getBytes(UTF_8)),
ByteBuffer.wrap(newTableName.getBytes(UTF_8)));
Map<String,String> opts = new HashMap<>();
@@ -1498,6 +1504,8 @@ public class TableOperationsImpl extends TableOperationsHelper {
throws TableExistsException, AccumuloException, AccumuloSecurityException {
checkArgument(tableName != null, "tableName is null");
checkArgument(importDir != null, "importDir is null");
+ checkArgument(tableName.length() <= MAX_TABLE_NAME_LEN,
+ "Table name is longer than " + MAX_TABLE_NAME_LEN + " characters");
try {
importDir = checkPath(importDir, "Table", "").toString();
diff --git a/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java b/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java
index 0373b71..8ffed95 100644
--- a/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java
+++ b/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java
@@ -18,6 +18,8 @@
*/
package org.apache.accumulo.master;
+import static org.apache.accumulo.core.Constants.MAX_NAMESPACE_LEN;
+import static org.apache.accumulo.core.Constants.MAX_TABLE_NAME_LEN;
import static org.apache.accumulo.master.util.TableValidators.CAN_CLONE;
import static org.apache.accumulo.master.util.TableValidators.NOT_METADATA;
import static org.apache.accumulo.master.util.TableValidators.NOT_ROOT_ID;
@@ -113,7 +115,7 @@ class FateServiceHandler implements FateService.Iface {
case NAMESPACE_CREATE: {
TableOperation tableOp = TableOperation.CREATE;
validateArgumentCount(arguments, tableOp, 1);
- String namespace = validateNamespaceArgument(arguments.get(0), tableOp, null);
+ String namespace = validateNewNamespaceArgument(arguments.get(0), tableOp, null);
if (!master.security.canCreateNamespace(c))
throw new ThriftSecurityException(c.getPrincipal(), SecurityErrorCode.PERMISSION_DENIED);
@@ -128,7 +130,7 @@ class FateServiceHandler implements FateService.Iface {
validateArgumentCount(arguments, tableOp, 2);
String oldName = validateNamespaceArgument(arguments.get(0), tableOp,
Namespaces.NOT_DEFAULT.and(Namespaces.NOT_ACCUMULO));
- String newName = validateNamespaceArgument(arguments.get(1), tableOp, null);
+ String newName = validateNewNamespaceArgument(arguments.get(1), tableOp, null);
NamespaceId namespaceId =
ClientServiceHandler.checkNamespaceId(master.getContext(), oldName, tableOp);
@@ -162,7 +164,7 @@ class FateServiceHandler implements FateService.Iface {
TableOperationExceptionType.OTHER,
"Expected at least " + SPLIT_OFFSET + " arguments, saw :" + arguments.size());
}
- String tableName = validateTableNameArgument(arguments.get(0), tableOp, NOT_SYSTEM);
+ String tableName = validateNewTableNameArgument(arguments.get(0), tableOp, NOT_SYSTEM);
TimeType timeType = TimeType.valueOf(ByteBufferUtil.toString(arguments.get(1)));
InitialTableState initialTableState =
InitialTableState.valueOf(ByteBufferUtil.toString(arguments.get(2)));
@@ -206,7 +208,8 @@ class FateServiceHandler implements FateService.Iface {
final String oldTableName =
validateTableNameArgument(arguments.get(0), tableOp, NOT_SYSTEM);
String newTableName =
- validateTableNameArgument(arguments.get(1), tableOp, new Validator<>() {
+
+ validateNewTableNameArgument(arguments.get(1), tableOp, new Validator<>() {
@Override
public boolean test(String argument) {
@@ -254,7 +257,7 @@ class FateServiceHandler implements FateService.Iface {
TableOperation tableOp = TableOperation.CLONE;
validateArgumentCount(arguments, tableOp, 3);
TableId srcTableId = validateTableIdArgument(arguments.get(0), tableOp, CAN_CLONE);
- String tableName = validateTableNameArgument(arguments.get(1), tableOp, NOT_SYSTEM);
+ String tableName = validateNewTableNameArgument(arguments.get(1), tableOp, NOT_SYSTEM);
boolean keepOffline = false;
if (arguments.get(2) != null) {
keepOffline = Boolean.parseBoolean(ByteBufferUtil.toString(arguments.get(2)));
@@ -512,7 +515,7 @@ class FateServiceHandler implements FateService.Iface {
case TABLE_IMPORT: {
TableOperation tableOp = TableOperation.IMPORT;
validateArgumentCount(arguments, tableOp, 2);
- String tableName = validateTableNameArgument(arguments.get(0), tableOp, NOT_SYSTEM);
+ String tableName = validateNewTableNameArgument(arguments.get(0), tableOp, NOT_SYSTEM);
String exportDir = ByteBufferUtil.toString(arguments.get(1));
NamespaceId namespaceId;
try {
@@ -697,10 +700,29 @@ class FateServiceHandler implements FateService.Iface {
}
}
- // Verify table name arguments are valid, and match any additional restrictions
+ // Verify existing table's name argument is valid, and match any additional restrictions
private String validateTableNameArgument(ByteBuffer tableNameArg, TableOperation op,
Validator<String> userValidator) throws ThriftTableOperationException {
String tableName = tableNameArg == null ? null : ByteBufferUtil.toString(tableNameArg);
+ if ((tableName != null) && (tableName.length() > MAX_TABLE_NAME_LEN)) {
+ log.warn("Table names greater than " + MAX_TABLE_NAME_LEN
+ + " characters should be renamed to conform to a " + MAX_TABLE_NAME_LEN
+ + " character limit. Longer table names are no longer supported and may result in "
+ + " unexpected behavior.");
+ }
+ return _validateArgument(tableName, op, VALID_NAME.and(userValidator));
+ }
+
+ // Verify table name arguments are valid, and match any additional restrictions
+ private String validateNewTableNameArgument(ByteBuffer tableNameArg, TableOperation op,
+ Validator<String> userValidator) throws ThriftTableOperationException {
+ String tableName = tableNameArg == null ? null : ByteBufferUtil.toString(tableNameArg);
+ if ((tableName != null) && (tableName.length() > MAX_TABLE_NAME_LEN)) {
+ throw new ThriftTableOperationException(null, tableName, op,
+ TableOperationExceptionType.INVALID_NAME,
+ "Table names must be less than or equal to " + MAX_TABLE_NAME_LEN + " characters. " + "'"
+ + tableName + "' is " + tableName.length() + " characters long.");
+ }
return _validateArgument(tableName, op, VALID_NAME.and(userValidator));
}
@@ -716,6 +738,25 @@ class FateServiceHandler implements FateService.Iface {
private String validateNamespaceArgument(ByteBuffer namespaceArg, TableOperation op,
Validator<String> userValidator) throws ThriftTableOperationException {
String namespace = namespaceArg == null ? null : ByteBufferUtil.toString(namespaceArg);
+ if ((namespace != null) && (namespace.length() > MAX_NAMESPACE_LEN)) {
+ log.warn("Namespaces greater than " + MAX_NAMESPACE_LEN
+ + " characters should be renamed to conform to a " + MAX_NAMESPACE_LEN
+ + " character limit. "
+ + "Longer namespaces are no longer supported and may result in unexpected behavior.");
+ }
+ return _validateArgument(namespace, op, Namespaces.VALID_NAME.and(userValidator));
+ }
+
+ // Verify namespace arguments are valid, and match any additional restrictions
+ private String validateNewNamespaceArgument(ByteBuffer namespaceArg, TableOperation op,
+ Validator<String> userValidator) throws ThriftTableOperationException {
+ String namespace = namespaceArg == null ? null : ByteBufferUtil.toString(namespaceArg);
+ if ((namespace != null) && (namespace.length() > MAX_NAMESPACE_LEN)) {
+ throw new ThriftTableOperationException(null, namespace, op,
+ TableOperationExceptionType.INVALID_NAME,
+ "Namespaces must be less than or equal to " + MAX_NAMESPACE_LEN + " characters. " + "'"
+ + namespace + "' is " + namespace.length() + " characters long.");
+ }
return _validateArgument(namespace, op, Namespaces.VALID_NAME.and(userValidator));
}
diff --git a/test/src/main/java/org/apache/accumulo/test/NamespacesIT.java b/test/src/main/java/org/apache/accumulo/test/NamespacesIT.java
index 9a5ef2c..9cafe04 100644
--- a/test/src/main/java/org/apache/accumulo/test/NamespacesIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/NamespacesIT.java
@@ -18,6 +18,7 @@
*/
package org.apache.accumulo.test;
+import static org.apache.accumulo.core.Constants.MAX_NAMESPACE_LEN;
import static org.apache.accumulo.fate.util.UtilWaitThread.sleepUninterruptibly;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
@@ -192,6 +193,22 @@ public class NamespacesIT extends SharedMiniClusterBase {
}
@Test
+ public void createNamespaceWithNamespaceLengthLimit()
+ throws AccumuloException, AccumuloSecurityException, NamespaceExistsException {
+ StringBuilder namespaceBuilder = new StringBuilder();
+ for (int i = 0; i <= MAX_NAMESPACE_LEN; i++) {
+ namespaceBuilder.append('a');
+ }
+ String namespace = namespaceBuilder.toString();
+ try {
+ c.namespaceOperations().create(namespace);
+ fail("IllegalArgumentException was not thrown");
+ } catch (IllegalArgumentException exc) {
+ assertTrue(!c.namespaceOperations().exists(namespace));
+ }
+ }
+
+ @Test
public void createAndDeleteNamespace() throws Exception {
String t1 = namespace + ".1";
String t2 = namespace + ".2";
diff --git a/test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java b/test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java
index 632a905..be9a0ab 100644
--- a/test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java
@@ -18,8 +18,11 @@
*/
package org.apache.accumulo.test;
+import static org.apache.accumulo.core.Constants.MAX_TABLE_NAME_LEN;
import static org.apache.accumulo.fate.util.UtilWaitThread.sleepUninterruptibly;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@@ -77,7 +80,7 @@ public class TableOperationsIT extends AccumuloClusterHarness {
@Override
public int defaultTimeoutSeconds() {
- return 30;
+ return 90;
}
@Before
@@ -104,16 +107,12 @@ public class TableOperationsIT extends AccumuloClusterHarness {
accumuloClient.securityOperations().revokeTablePermission(getAdminPrincipal(), tableName,
TablePermission.READ);
- try {
- accumuloClient.tableOperations().getDiskUsage(Collections.singleton(tableName));
- fail("Should throw securityexception");
- } catch (AccumuloSecurityException e) {}
+ assertThrows(AccumuloSecurityException.class,
+ () -> accumuloClient.tableOperations().getDiskUsage(Collections.singleton(tableName)));
accumuloClient.tableOperations().delete(tableName);
- try {
- accumuloClient.tableOperations().getDiskUsage(Collections.singleton(tableName));
- fail("Should throw tablenotfound");
- } catch (TableNotFoundException e) {}
+ assertThrows(TableNotFoundException.class,
+ () -> accumuloClient.tableOperations().getDiskUsage(Collections.singleton(tableName)));
}
@Test
@@ -190,6 +189,19 @@ public class TableOperationsIT extends AccumuloClusterHarness {
}
@Test
+ public void createTableWithTableNameLengthLimit()
+ throws AccumuloException, AccumuloSecurityException, TableExistsException {
+ StringBuilder tableNameBuilder = new StringBuilder();
+ for (int i = 0; i <= MAX_TABLE_NAME_LEN; i++) {
+ tableNameBuilder.append('a');
+ }
+ String tableName = tableNameBuilder.toString();
+ assertThrows(IllegalArgumentException.class,
+ () -> accumuloClient.tableOperations().create(tableName));
+ assertFalse(accumuloClient.tableOperations().exists(tableName));
+ }
+
+ @Test
public void createMergeClonedTable() throws Exception {
String[] names = getUniqueNames(2);
String originalTable = names[0];