HBASE-11275 [AccessController] postCreateTable hook fails when another CP creates table on their startup (Anoop Sam John)

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

@@ -833,6 +833,8 @@ public class HMaster extends HRegionServer implements MasterServices, Server {
    // We depend on there being only one instance of this executor running
    // at a time.  To do concurrency, would need fencing of enable/disable of
    // tables.
+   // Any time changing this maxThreads to > 1, pls see the comment at
+   // AccessController#postCreateTableHandler
    this.service.startExecutorService(ExecutorType.MASTER_TABLE_OPERATIONS, 1);
 
    // Start log cleaner thread

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java

@@ -43,7 +43,6 @@ import org.apache.hadoop.hbase.NamespaceDescriptor;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.Tag;
 import org.apache.hadoop.hbase.TagType;
-import org.apache.hadoop.hbase.catalog.MetaReader;
 import org.apache.hadoop.hbase.client.Delete;
 import org.apache.hadoop.hbase.client.Get;
 import org.apache.hadoop.hbase.client.HTable;
@@ -135,9 +134,7 @@ public class AccessControlLists {
    * @param master reference to HMaster
    */
   static void init(MasterServices master) throws IOException {
-    if (!MetaReader.tableExists(master.getCatalogTracker(), ACL_TABLE_NAME)) {
+    master.createTable(ACL_TABLEDESC, null);
-      master.createTable(ACL_TABLEDESC, null);
-    }
   }
 
   /**

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java

@@ -48,6 +48,7 @@ import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.TableNotDisabledException;
 import org.apache.hadoop.hbase.TableNotFoundException;
 import org.apache.hadoop.hbase.Tag;
+import org.apache.hadoop.hbase.catalog.MetaReader;
 import org.apache.hadoop.hbase.client.Append;
 import org.apache.hadoop.hbase.client.Delete;
 import org.apache.hadoop.hbase.client.Durability;
@@ -178,6 +179,9 @@ public class AccessController extends BaseRegionObserver
 
   private volatile boolean initialized = false;
 
+  // This boolean having relevance only in the Master.
+  private volatile boolean aclTabAvailable = false;
+
   public HRegion getRegion() {
     return regionEnv != null ? regionEnv.getRegion() : null;
   }
@@ -837,20 +841,44 @@ public class AccessController extends BaseRegionObserver
 
   @Override
   public void postCreateTable(ObserverContext<MasterCoprocessorEnvironment> c,
-      HTableDescriptor desc, HRegionInfo[] regions) throws IOException {
+      HTableDescriptor desc, HRegionInfo[] regions) throws IOException {}
-    if (!AccessControlLists.isAclTable(desc)) {
-      String owner = desc.getOwnerString();
-      // default the table owner to current user, if not specified.
-      if (owner == null) owner = getActiveUser().getShortName();
-      UserPermission userperm = new UserPermission(Bytes.toBytes(owner), desc.getTableName(), null,
-          Action.values());
-      AccessControlLists.addUserPermission(c.getEnvironment().getConfiguration(), userperm);
-    }
-  }
 
   @Override
   public void postCreateTableHandler(ObserverContext<MasterCoprocessorEnvironment> c,
-      HTableDescriptor desc, HRegionInfo[] regions) throws IOException {}
+      HTableDescriptor desc, HRegionInfo[] regions) throws IOException {
+    // When AC is used, it should be configured as the 1st CP.
+    // In Master, the table operations like create, are handled by a Thread pool but the max size
+    // for this pool is 1. So if multiple CPs create tables on startup, these creations will happen
+    // sequentially only.
+    // Related code in HMaster#startServiceThreads
+    // {code}
+    //   // We depend on there being only one instance of this executor running
+    //   // at a time. To do concurrency, would need fencing of enable/disable of
+    //   // tables.
+    //   this.service.startExecutorService(ExecutorType.MASTER_TABLE_OPERATIONS, 1);
+    // {code}
+    // In future if we change this pool to have more threads, then there is a chance for thread,
+    // creating acl table, getting delayed and by that time another table creation got over and
+    // this hook is getting called. In such a case, we will need a wait logic here which will
+    // wait till the acl table is created.
+    if (AccessControlLists.isAclTable(desc)) {
+      this.aclTabAvailable = true;
+    } else if (!(TableName.NAMESPACE_TABLE_NAME.equals(desc.getTableName()))) {
+      if (!aclTabAvailable) {
+        LOG.warn("Not adding owner permission for table " + desc.getTableName() + ". "
+            + AccessControlLists.ACL_TABLE_NAME + " is not yet created. "
+            + getClass().getSimpleName() + " should be configured as the first Coprocessor");
+      } else {
+        String owner = desc.getOwnerString();
+        // default the table owner to current user, if not specified.
+        if (owner == null)
+          owner = getActiveUser().getShortName();
+        UserPermission userperm = new UserPermission(Bytes.toBytes(owner), desc.getTableName(),
+            null, Action.values());
+        AccessControlLists.addUserPermission(c.getEnvironment().getConfiguration(), userperm);
+      }
+    }
+  }
 
   @Override
   public void preDeleteTable(ObserverContext<MasterCoprocessorEnvironment> c, TableName tableName)
@@ -1088,8 +1116,13 @@ public class AccessController extends BaseRegionObserver
   @Override
   public void postStartMaster(ObserverContext<MasterCoprocessorEnvironment> ctx)
       throws IOException {
-    // initialize the ACL storage table
+    if (!MetaReader.tableExists(ctx.getEnvironment().getMasterServices().getCatalogTracker(),
-    AccessControlLists.init(ctx.getEnvironment().getMasterServices());
+        AccessControlLists.ACL_TABLE_NAME)) {
+      // initialize the ACL storage table
+      AccessControlLists.init(ctx.getEnvironment().getMasterServices());
+    } else {
+      aclTabAvailable = true;
+    }
   }
 
   @Override