From db36cf71c57f45908b835d06f9c4769c7080921d Mon Sep 17 00:00:00 2001
From: Reid Chan <reidchan@apache.org>
Date: Sat, 18 Apr 2020 01:24:31 +0800
Subject: [PATCH] HBASE-24174 Fix findbugs warning on
 ServiceAuthorizationManager for master branch (#1537)

Signed-off-by: binlijin <binlijin@gmail.com>
Signed-off-by: Viraj Jasani <vjasani@apache.org>
---
 .../apache/hadoop/hbase/ipc/RpcServer.java    | 27 +++++++------------
 .../hadoop/hbase/ipc/RpcServerInterface.java  |  3 ++-
 .../token/TestTokenAuthentication.java        |  2 +-
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
index 6b993fe295f..b6c004f27bf 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
@@ -122,8 +122,6 @@ public abstract class RpcServer implements RpcServerInterface,
   protected SecretManager<TokenIdentifier> secretManager;
   protected final Map<String, String> saslProps;
 
-  @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="IS2_INCONSISTENT_SYNC",
-      justification="Start is synchronized so authManager creation is single-threaded")
   protected ServiceAuthorizationManager authManager;
 
   /** This is set to Call object before Handler invokes an RPC and ybdie
@@ -315,14 +313,9 @@ public abstract class RpcServer implements RpcServerInterface,
     if (scheduler instanceof ConfigurationObserver) {
       ((ConfigurationObserver) scheduler).onConfigurationChange(newConf);
     }
-    // Make sure authManager will read hbase-policy file
-    System.setProperty("hadoop.policy.file", "hbase-policy.xml");
-    synchronized (authManager) {
-      authManager.refresh(newConf, new HBasePolicyProvider());
+    if (authorize) {
+      refreshAuthManager(newConf, new HBasePolicyProvider());
     }
-    LOG.info("Refreshed hbase-policy.xml successfully");
-    ProxyUsers.refreshSuperUserGroupsConfiguration(newConf);
-    LOG.info("Refreshed super and proxy users successfully");
   }
 
   protected void initReconfigurable(Configuration confToLoad) {
@@ -349,12 +342,14 @@ public abstract class RpcServer implements RpcServerInterface,
   }
 
   @Override
-  public void refreshAuthManager(PolicyProvider pp) {
+  public synchronized void refreshAuthManager(Configuration conf, PolicyProvider pp) {
     // Ignore warnings that this should be accessed in a static way instead of via an instance;
     // it'll break if you go via static route.
-    synchronized (authManager) {
-      authManager.refresh(this.conf, pp);
-    }
+    System.setProperty("hadoop.policy.file", "hbase-policy.xml");
+    this.authManager.refresh(conf, pp);
+    LOG.info("Refreshed hbase-policy.xml successfully");
+    ProxyUsers.refreshSuperUserGroupsConfiguration(conf);
+    LOG.info("Refreshed super and proxy users successfully");
   }
 
   protected AuthenticationTokenSecretManager createSecretManager() {
@@ -597,13 +592,11 @@ public abstract class RpcServer implements RpcServerInterface,
    * @param addr InetAddress of incoming connection
    * @throws AuthorizationException when the client isn't authorized to talk the protocol
    */
-  public void authorize(UserGroupInformation user, ConnectionHeader connection,
+  public synchronized void authorize(UserGroupInformation user, ConnectionHeader connection,
       InetAddress addr) throws AuthorizationException {
     if (authorize) {
       Class<?> c = getServiceInterface(services, connection.getServiceName());
-      synchronized (authManager) {
-        authManager.authorize(user, c, getConf(), addr);
-      }
+      authManager.authorize(user, c, getConf(), addr);
     }
   }
 
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServerInterface.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServerInterface.java
index 31556796697..c8a71f3acef 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServerInterface.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServerInterface.java
@@ -22,6 +22,7 @@ package org.apache.hadoop.hbase.ipc;
 import java.io.IOException;
 import java.net.InetSocketAddress;
 
+import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.CellScanner;
 import org.apache.hadoop.hbase.io.ByteBuffAllocator;
 import org.apache.hadoop.hbase.monitoring.MonitoredRPCHandler;
@@ -86,7 +87,7 @@ public interface RpcServerInterface {
    * @param pp
    */
   @VisibleForTesting
-  void refreshAuthManager(PolicyProvider pp);
+  void refreshAuthManager(Configuration conf, PolicyProvider pp);
 
   RpcScheduler getScheduler();
 
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/token/TestTokenAuthentication.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/token/TestTokenAuthentication.java
index 0493e39c449..9131370906e 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/token/TestTokenAuthentication.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/token/TestTokenAuthentication.java
@@ -403,7 +403,7 @@ public class TestTokenAuthentication {
     while (!server.isStarted() && !server.isStopped()) {
       Thread.sleep(10);
     }
-    server.rpcServer.refreshAuthManager(new PolicyProvider() {
+    server.rpcServer.refreshAuthManager(conf, new PolicyProvider() {
       @Override
       public Service[] getServices() {
         return new Service [] {