HBASE-19048 Cleanup MasterObserver hooks which takes IA private params

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java

@@ -1,5 +1,4 @@
 /*
- *
  * 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
@@ -23,26 +22,18 @@ import java.io.IOException;
 import java.util.List;
 import java.util.Set;
 
-import org.apache.hadoop.hbase.Coprocessor;
 import org.apache.hadoop.hbase.HBaseInterfaceAudience;
 import org.apache.hadoop.hbase.MetaMutationAnnotation;
 import org.apache.hadoop.hbase.NamespaceDescriptor;
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.TableName;
-import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
 import org.apache.hadoop.hbase.client.MasterSwitchType;
 import org.apache.hadoop.hbase.client.Mutation;
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.client.SnapshotDescription;
 import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.master.RegionPlan;
-import org.apache.hadoop.hbase.master.locking.LockProcedure;
-import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.net.Address;
-import org.apache.hadoop.hbase.procedure2.LockType;
-import org.apache.hadoop.hbase.procedure2.LockedResource;
-import org.apache.hadoop.hbase.procedure2.Procedure;
-import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
 import org.apache.hadoop.hbase.quotas.GlobalQuotaSettings;
 import org.apache.hadoop.hbase.replication.ReplicationPeerConfig;
 import org.apache.yetus.audience.InterfaceAudience;
@@ -356,13 +347,10 @@ public interface MasterObserver {
   /**
    * Called before a abortProcedure request has been processed.
    * @param ctx the environment to interact with the framework and master
-   * @param procEnv procedure executor
    * @param procId the Id of the procedure
    */
   default void preAbortProcedure(
-      ObserverContext<MasterCoprocessorEnvironment> ctx,
+      ObserverContext<MasterCoprocessorEnvironment> ctx, final long procId) throws IOException {}
-      final ProcedureExecutor<MasterProcedureEnv> procEnv,
-      final long procId) throws IOException {}
 
   /**
    * Called after a abortProcedure request has been processed.
@@ -381,11 +369,9 @@ public interface MasterObserver {
   /**
    * Called after a getProcedures request has been processed.
    * @param ctx the environment to interact with the framework and master
-   * @param procList the list of procedures about to be returned
    */
-  default void postGetProcedures(
+  default void postGetProcedures(ObserverContext<MasterCoprocessorEnvironment> ctx)
-      ObserverContext<MasterCoprocessorEnvironment> ctx,
+      throws IOException {}
-      List<Procedure<?>> procList) throws IOException {}
 
   /**
    * Called before a getLocks request has been processed.
@@ -398,12 +384,10 @@ public interface MasterObserver {
   /**
    * Called after a getLocks request has been processed.
    * @param ctx the environment to interact with the framework and master
-   * @param lockedResources the list of locks about to be returned
    * @throws IOException if something went wrong
    */
   default void postGetLocks(
-      ObserverContext<MasterCoprocessorEnvironment> ctx,
+      ObserverContext<MasterCoprocessorEnvironment> ctx) throws IOException {}
-      List<LockedResource> lockedResources) throws IOException {}
 
   /**
    * Called prior to moving a given region from one region server to another.
@@ -1269,32 +1253,28 @@ public interface MasterObserver {
    * @param ctx the environment to interact with the framework and master
    */
   default void preRequestLock(ObserverContext<MasterCoprocessorEnvironment> ctx, String namespace,
-      TableName tableName, RegionInfo[] regionInfos, LockType type,
+      TableName tableName, RegionInfo[] regionInfos, String description) throws IOException {}
-      String description) throws IOException {}
 
   /**
    * Called after new LockProcedure is queued.
    * @param ctx the environment to interact with the framework and master
    */
   default void postRequestLock(ObserverContext<MasterCoprocessorEnvironment> ctx, String namespace,
-      TableName tableName, RegionInfo[] regionInfos, LockType type,
+      TableName tableName, RegionInfo[] regionInfos, String description) throws IOException {}
-      String description) throws IOException {}
 
   /**
    * Called before heartbeat to a lock.
    * @param ctx the environment to interact with the framework and master
-   * @param keepAlive if lock should be kept alive; lock will be released if set to false.
    */
   default void preLockHeartbeat(ObserverContext<MasterCoprocessorEnvironment> ctx,
-      LockProcedure proc, boolean keepAlive) throws IOException {}
+      TableName tn, String description) throws IOException {}
 
   /**
    * Called after heartbeat to a lock.
    * @param ctx the environment to interact with the framework and master
-   * @param keepAlive if lock was kept alive; lock was released if set to false.
    */
-  default void postLockHeartbeat(ObserverContext<MasterCoprocessorEnvironment> ctx,
+  default void postLockHeartbeat(ObserverContext<MasterCoprocessorEnvironment> ctx)
-      LockProcedure proc, boolean keepAlive) throws IOException {}
+      throws IOException {}
 
   /**
    * Called before list dead region servers.

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

@@ -1,5 +1,4 @@
 /*
- *
  * 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
@@ -31,7 +30,6 @@ import org.apache.hadoop.hbase.MetaMutationAnnotation;
 import org.apache.hadoop.hbase.NamespaceDescriptor;
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.TableName;
-import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
 import org.apache.hadoop.hbase.client.Connection;
 import org.apache.hadoop.hbase.client.MasterSwitchType;
 import org.apache.hadoop.hbase.client.Mutation;
@@ -536,7 +534,7 @@ public class MasterCoprocessorHost
     return execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
       @Override
       public void call(MasterObserver observer) throws IOException {
-        observer.preAbortProcedure(this, procEnv, procId);
+        observer.preAbortProcedure(this,  procId);
       }
     });
   }
@@ -563,7 +561,7 @@ public class MasterCoprocessorHost
     execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
       @Override
       public void call(MasterObserver observer) throws IOException {
-        observer.postGetProcedures(this, procInfoList);
+        observer.postGetProcedures(this);
       }
     });
   }
@@ -581,7 +579,7 @@ public class MasterCoprocessorHost
     execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
       @Override
       public void call(MasterObserver observer) throws IOException {
-        observer.postGetLocks(this, lockedResources);
+        observer.postGetLocks(this);
       }
     });
   }
@@ -1517,7 +1515,7 @@ public class MasterCoprocessorHost
     execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
       @Override
       public void call(MasterObserver observer) throws IOException {
-        observer.preRequestLock(this, namespace, tableName, regionInfos, type, description);
+        observer.preRequestLock(this, namespace, tableName, regionInfos, description);
       }
     });
   }
@@ -1527,7 +1525,7 @@ public class MasterCoprocessorHost
     execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
       @Override
       public void call(MasterObserver observer) throws IOException {
-        observer.postRequestLock(this, namespace, tableName, regionInfos, type, description);
+        observer.postRequestLock(this, namespace, tableName, regionInfos, description);
       }
     });
   }
@@ -1536,7 +1534,7 @@ public class MasterCoprocessorHost
     execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
       @Override
       public void call(MasterObserver observer) throws IOException {
-        observer.preLockHeartbeat(this, proc, keepAlive);
+        observer.preLockHeartbeat(this, proc.getTableName(), proc.getDescription());
       }
     });
   }
@@ -1545,7 +1543,7 @@ public class MasterCoprocessorHost
     execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() {
       @Override
       public void call(MasterObserver observer) throws IOException {
-        observer.postLockHeartbeat(this, proc, keepAlive);
+        observer.postLockHeartbeat(this);
       }
     });
   }

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

@@ -99,12 +99,7 @@ import org.apache.hadoop.hbase.filter.FilterList;
 import org.apache.hadoop.hbase.io.hfile.HFile;
 import org.apache.hadoop.hbase.ipc.CoprocessorRpcUtils;
 import org.apache.hadoop.hbase.ipc.RpcServer;
-import org.apache.hadoop.hbase.master.locking.LockProcedure;
-import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.net.Address;
-import org.apache.hadoop.hbase.procedure2.LockType;
-import org.apache.hadoop.hbase.procedure2.Procedure;
-import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
 import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.protobuf.generated.AccessControlProtos;
 import org.apache.hadoop.hbase.protobuf.generated.AccessControlProtos.AccessControlService;
@@ -1218,15 +1213,9 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
   }
 
   @Override
-  public void preAbortProcedure(
+  public void preAbortProcedure(ObserverContext<MasterCoprocessorEnvironment> ctx,
-      ObserverContext<MasterCoprocessorEnvironment> ctx,
-      final ProcedureExecutor<MasterProcedureEnv> procEnv,
       final long procId) throws IOException {
-    if (!procEnv.isProcedureOwner(procId, getActiveUser(ctx))) {
+    requirePermission(getActiveUser(ctx), "abortProcedure", Action.ADMIN);
-      // If the user is not the procedure owner, then we should further probe whether
-      // he can abort the procedure.
-      requirePermission(getActiveUser(ctx), "abortProcedure", Action.ADMIN);
-    }
   }
 
   @Override
@@ -1238,35 +1227,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
   @Override
   public void preGetProcedures(ObserverContext<MasterCoprocessorEnvironment> ctx)
       throws IOException {
-    // We are delegating the authorization check to postGetProcedures as we don't have
+    requirePermission(getActiveUser(ctx), "getProcedure", Action.ADMIN);
-    // any concrete set of procedures to work with
-  }
-
-  @Override
-  public void postGetProcedures(
-      ObserverContext<MasterCoprocessorEnvironment> ctx,
-      List<Procedure<?>> procList) throws IOException {
-    if (procList.isEmpty()) {
-      return;
-    }
-
-    // Retains only those which passes authorization checks, as the checks weren't done as part
-    // of preGetProcedures.
-    Iterator<Procedure<?>> itr = procList.iterator();
-    User user = getActiveUser(ctx);
-    while (itr.hasNext()) {
-      Procedure<?> proc = itr.next();
-      try {
-        String owner = proc.getOwner();
-        if (owner == null || !owner.equals(user.getShortName())) {
-          // If the user is not the procedure owner, then we should further probe whether
-          // he can see the procedure.
-          requirePermission(user, "getProcedures", Action.ADMIN);
-        }
-      } catch (AccessDeniedException e) {
-        itr.remove();
-      }
-    }
   }
 
   @Override
@@ -2787,19 +2748,18 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
 
   @Override
   public void preRequestLock(ObserverContext<MasterCoprocessorEnvironment> ctx, String namespace,
-      TableName tableName, RegionInfo[] regionInfos, LockType type, String description)
+      TableName tableName, RegionInfo[] regionInfos, String description)
   throws IOException {
     // There are operations in the CREATE and ADMIN domain which may require lock, READ
     // or WRITE. So for any lock request, we check for these two perms irrespective of lock type.
-    String reason = String.format("Lock %s, description=%s", type, description);
+    String reason = String.format("Description=%s", description);
     checkLockPermissions(getActiveUser(ctx), namespace, tableName, regionInfos, reason);
   }
 
   @Override
   public void preLockHeartbeat(ObserverContext<MasterCoprocessorEnvironment> ctx,
-      LockProcedure proc, boolean keepAlive) throws IOException {
+      TableName tableName, String description) throws IOException {
-    String reason = "Heartbeat for lock " + proc.getProcId();
+    checkLockPermissions(getActiveUser(ctx), null, tableName, null, description);
-    checkLockPermissions(getActiveUser(ctx), null, proc.getTableName(), null, reason);
   }
 
   private void checkLockPermissions(User user, String namespace,

hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java

@@ -1,5 +1,4 @@
 /*
- *
  * 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
@@ -56,13 +55,8 @@ import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.master.HMaster;
 import org.apache.hadoop.hbase.master.MasterCoprocessorHost;
 import org.apache.hadoop.hbase.master.RegionPlan;
-import org.apache.hadoop.hbase.master.locking.LockProcedure;
-import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.net.Address;
 import org.apache.hadoop.hbase.procedure2.LockType;
-import org.apache.hadoop.hbase.procedure2.LockedResource;
-import org.apache.hadoop.hbase.procedure2.Procedure;
-import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
 import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility;
 import org.apache.hadoop.hbase.quotas.GlobalQuotaSettings;
 import org.apache.hadoop.hbase.regionserver.HRegionServer;
@@ -564,9 +558,7 @@ public class TestMasterObserver {
 
     @Override
     public void preAbortProcedure(
-        ObserverContext<MasterCoprocessorEnvironment> ctx,
+        ObserverContext<MasterCoprocessorEnvironment> ctx, final long procId) throws IOException {
-        final ProcedureExecutor<MasterProcedureEnv> procEnv,
-        final long procId) throws IOException {
       preAbortProcedureCalled = true;
     }
 
@@ -592,8 +584,7 @@ public class TestMasterObserver {
 
     @Override
     public void postGetProcedures(
-        ObserverContext<MasterCoprocessorEnvironment> ctx,
+        ObserverContext<MasterCoprocessorEnvironment> ctx) throws IOException {
-        List<Procedure<?>> procInfoList) throws IOException {
       postGetProceduresCalled = true;
     }
 
@@ -611,7 +602,7 @@ public class TestMasterObserver {
     }
 
     @Override
-    public void postGetLocks(ObserverContext<MasterCoprocessorEnvironment> ctx, List<LockedResource> lockedResources)
+    public void postGetLocks(ObserverContext<MasterCoprocessorEnvironment> ctx)
         throws IOException {
       postGetLocksCalled = true;
     }
@@ -1220,27 +1211,25 @@ public class TestMasterObserver {
 
     @Override
     public void preRequestLock(ObserverContext<MasterCoprocessorEnvironment> ctx, String namespace,
-        TableName tableName, RegionInfo[] regionInfos, LockType type,
+        TableName tableName, RegionInfo[] regionInfos, String description) throws IOException {
-        String description) throws IOException {
       preRequestLockCalled = true;
     }
 
     @Override
     public void postRequestLock(ObserverContext<MasterCoprocessorEnvironment> ctx, String namespace,
-        TableName tableName, RegionInfo[] regionInfos, LockType type,
+        TableName tableName, RegionInfo[] regionInfos, String description) throws IOException {
-        String description) throws IOException {
       postRequestLockCalled = true;
     }
 
     @Override
     public void preLockHeartbeat(ObserverContext<MasterCoprocessorEnvironment> ctx,
-        LockProcedure proc, boolean keepAlive) throws IOException {
+        TableName tn, String description) throws IOException {
       preLockHeartbeatCalled = true;
     }
 
     @Override
-    public void postLockHeartbeat(ObserverContext<MasterCoprocessorEnvironment> ctx,
+    public void postLockHeartbeat(ObserverContext<MasterCoprocessorEnvironment> ctx)
-        LockProcedure proc, boolean keepAlive) throws IOException {
+        throws IOException {
       postLockHeartbeatCalled = true;
     }
 

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

@@ -127,6 +127,7 @@ import org.apache.hadoop.hbase.testclassification.SecurityTests;
 import org.apache.hadoop.hbase.tool.LoadIncrementalHFiles;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.JVMClusterUtil;
+import org.apache.hadoop.hbase.util.Threads;
 import org.apache.log4j.Level;
 import org.apache.log4j.Logger;
 import org.junit.AfterClass;
@@ -553,26 +554,16 @@ public class TestAccessController extends SecureTestUtil {
 
   @Test
   public void testAbortProcedure() throws Exception {
-    final TableName tableName = TableName.valueOf(name.getMethodName());
+    long procId = 1;
-    final ProcedureExecutor<MasterProcedureEnv> procExec =
-        TEST_UTIL.getHBaseCluster().getMaster().getMasterProcedureExecutor();
-    Procedure proc = new TestTableDDLProcedure(procExec.getEnvironment(), tableName);
-    proc.setOwner(USER_OWNER);
-    final long procId = procExec.submitProcedure(proc);
-
     AccessTestAction abortProcedureAction = new AccessTestAction() {
       @Override
       public Object run() throws Exception {
-        ACCESS_CONTROLLER
+        ACCESS_CONTROLLER.preAbortProcedure(ObserverContextImpl.createAndPrepare(CP_ENV), procId);
-        .preAbortProcedure(ObserverContextImpl.createAndPrepare(CP_ENV), procExec, procId);
        return null;
       }
     };
 
     verifyAllowed(abortProcedureAction, SUPERUSER, USER_ADMIN, USER_GROUP_ADMIN);
-    verifyAllowed(abortProcedureAction, USER_OWNER);
-    verifyDenied(
-      abortProcedureAction, USER_RW, USER_RO, USER_NONE, USER_GROUP_READ, USER_GROUP_WRITE);
   }
 
   @Test
@@ -589,7 +580,7 @@ public class TestAccessController extends SecureTestUtil {
       @Override
       public Object run() throws Exception {
         ACCESS_CONTROLLER
-        .postGetProcedures(ObserverContextImpl.createAndPrepare(CP_ENV), procList);
+        .postGetProcedures(ObserverContextImpl.createAndPrepare(CP_ENV));
        return null;
       }
     };
@@ -3058,19 +3049,21 @@ public class TestAccessController extends SecureTestUtil {
     AccessTestAction namespaceLockAction = new AccessTestAction() {
       @Override public Object run() throws Exception {
         ACCESS_CONTROLLER.preRequestLock(ObserverContextImpl.createAndPrepare(CP_ENV), namespace,
-            null, null, LockType.EXCLUSIVE, null);
+            null, null, null);
         return null;
       }
     };
     verifyAllowed(namespaceLockAction, SUPERUSER, USER_ADMIN);
     verifyDenied(namespaceLockAction, globalRWXUser, tableACUser, namespaceUser, tableRWXUser);
     grantOnNamespace(TEST_UTIL, namespaceUser.getShortName(), namespace, Action.ADMIN);
+    // Why I need this pause? I don't need it elsewhere.
+    Threads.sleep(1000);
     verifyAllowed(namespaceLockAction, namespaceUser);
 
     AccessTestAction tableLockAction = new AccessTestAction() {
       @Override public Object run() throws Exception {
         ACCESS_CONTROLLER.preRequestLock(ObserverContextImpl.createAndPrepare(CP_ENV),
-            null, tableName, null, LockType.EXCLUSIVE, null);
+            null, tableName, null, null);
         return null;
       }
     };
@@ -3083,7 +3076,7 @@ public class TestAccessController extends SecureTestUtil {
     AccessTestAction regionsLockAction = new AccessTestAction() {
       @Override public Object run() throws Exception {
         ACCESS_CONTROLLER.preRequestLock(ObserverContextImpl.createAndPrepare(CP_ENV),
-            null, null, regionInfos, LockType.EXCLUSIVE, null);
+            null, null, regionInfos, null);
         return null;
       }
     };
@@ -3097,7 +3090,7 @@ public class TestAccessController extends SecureTestUtil {
     AccessTestAction regionLockHeartbeatAction = new AccessTestAction() {
       @Override public Object run() throws Exception {
         ACCESS_CONTROLLER.preLockHeartbeat(ObserverContextImpl.createAndPrepare(CP_ENV),
-            proc, false);
+            proc.getTableName(), proc.getDescription());
         return null;
       }
     };