From 69e0054853ba52c255eae2c1abf4e266eed2cb8a Mon Sep 17 00:00:00 2001 From: Michael Stack Date: Tue, 24 Oct 2017 12:46:02 -0700 Subject: [PATCH] HBASE-19048 Cleanup MasterObserver hooks which takes IA private params --- .../hbase/coprocessor/MasterObserver.java | 38 ++++--------- .../hbase/master/MasterCoprocessorHost.java | 16 +++--- .../security/access/AccessController.java | 54 +++---------------- .../hbase/coprocessor/TestMasterObserver.java | 27 +++------- .../security/access/TestAccessController.java | 27 ++++------ 5 files changed, 41 insertions(+), 121 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java index 397ec8ad4cb..2635c2ba347 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java +++ b/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 ctx, - final ProcedureExecutor procEnv, - final long procId) throws IOException {} + ObserverContext ctx, 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( - ObserverContext ctx, - List> procList) throws IOException {} + default void postGetProcedures(ObserverContext ctx) + 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 ctx, - List lockedResources) throws IOException {} + ObserverContext ctx) 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 ctx, String namespace, - TableName tableName, RegionInfo[] regionInfos, LockType type, - String description) throws IOException {} + TableName tableName, RegionInfo[] regionInfos, 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 ctx, String namespace, - TableName tableName, RegionInfo[] regionInfos, LockType type, - String description) throws IOException {} + TableName tableName, RegionInfo[] regionInfos, 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 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 ctx, - LockProcedure proc, boolean keepAlive) throws IOException {} + default void postLockHeartbeat(ObserverContext ctx) + throws IOException {} /** * Called before list dead region servers. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java index f3f34bf9af9..fa2a0a9dc63 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java +++ b/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); } }); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java index 1d1cf5b4bad..334d1edf5aa 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java +++ b/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( - ObserverContext ctx, - final ProcedureExecutor procEnv, + public void preAbortProcedure(ObserverContext ctx, final long procId) throws IOException { - if (!procEnv.isProcedureOwner(procId, getActiveUser(ctx))) { - // 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); - } + requirePermission(getActiveUser(ctx), "abortProcedure", Action.ADMIN); } @Override @@ -1238,35 +1227,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, @Override public void preGetProcedures(ObserverContext ctx) throws IOException { - // We are delegating the authorization check to postGetProcedures as we don't have - // any concrete set of procedures to work with - } - - @Override - public void postGetProcedures( - ObserverContext ctx, - List> 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> 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(); - } - } + requirePermission(getActiveUser(ctx), "getProcedure", Action.ADMIN); } @Override @@ -2787,19 +2748,18 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, @Override public void preRequestLock(ObserverContext 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 ctx, - LockProcedure proc, boolean keepAlive) throws IOException { - String reason = "Heartbeat for lock " + proc.getProcId(); - checkLockPermissions(getActiveUser(ctx), null, proc.getTableName(), null, reason); + TableName tableName, String description) throws IOException { + checkLockPermissions(getActiveUser(ctx), null, tableName, null, description); } private void checkLockPermissions(User user, String namespace, diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java index a1614db9812..e9a56bd9566 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java +++ b/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 ctx, - final ProcedureExecutor procEnv, - final long procId) throws IOException { + ObserverContext ctx, final long procId) throws IOException { preAbortProcedureCalled = true; } @@ -592,8 +584,7 @@ public class TestMasterObserver { @Override public void postGetProcedures( - ObserverContext ctx, - List> procInfoList) throws IOException { + ObserverContext ctx) throws IOException { postGetProceduresCalled = true; } @@ -611,7 +602,7 @@ public class TestMasterObserver { } @Override - public void postGetLocks(ObserverContext ctx, List lockedResources) + public void postGetLocks(ObserverContext ctx) throws IOException { postGetLocksCalled = true; } @@ -1220,27 +1211,25 @@ public class TestMasterObserver { @Override public void preRequestLock(ObserverContext ctx, String namespace, - TableName tableName, RegionInfo[] regionInfos, LockType type, - String description) throws IOException { + TableName tableName, RegionInfo[] regionInfos, String description) throws IOException { preRequestLockCalled = true; } @Override public void postRequestLock(ObserverContext ctx, String namespace, - TableName tableName, RegionInfo[] regionInfos, LockType type, - String description) throws IOException { + TableName tableName, RegionInfo[] regionInfos, String description) throws IOException { postRequestLockCalled = true; } @Override public void preLockHeartbeat(ObserverContext ctx, - LockProcedure proc, boolean keepAlive) throws IOException { + TableName tn, String description) throws IOException { preLockHeartbeatCalled = true; } @Override - public void postLockHeartbeat(ObserverContext ctx, - LockProcedure proc, boolean keepAlive) throws IOException { + public void postLockHeartbeat(ObserverContext ctx) + throws IOException { postLockHeartbeatCalled = true; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java index d196b64ae74..af789282ca9 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java +++ b/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()); - final ProcedureExecutor procExec = - TEST_UTIL.getHBaseCluster().getMaster().getMasterProcedureExecutor(); - Procedure proc = new TestTableDDLProcedure(procExec.getEnvironment(), tableName); - proc.setOwner(USER_OWNER); - final long procId = procExec.submitProcedure(proc); - + long procId = 1; AccessTestAction abortProcedureAction = new AccessTestAction() { @Override public Object run() throws Exception { - ACCESS_CONTROLLER - .preAbortProcedure(ObserverContextImpl.createAndPrepare(CP_ENV), procExec, procId); + ACCESS_CONTROLLER.preAbortProcedure(ObserverContextImpl.createAndPrepare(CP_ENV), 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; } };