Remove static local-node reference (#3066)

### Description Remove static reference/initialization of localNode variable inside security plugin, to fix `No user found..` errors caused due to mismatching localNode values in test. Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> (cherry picked from commit cca77be) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Aug 8, 2023 · de2f0ad · de2f0ad
1 parent cca01ac
commit de2f0ad
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 19 deletions.
diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
@@ -208,7 +208,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
     private volatile ConfigurationRepository cr;
     private volatile AdminDNs adminDns;
     private volatile ClusterService cs;
-    private static volatile DiscoveryNode localNode;
+    private volatile AtomicReference<DiscoveryNode> localNode = new AtomicReference<>();
     private volatile AuditLog auditLog;
     private volatile BackendRegistry backendRegistry;
     private volatile SslExceptionHandler sslExceptionHandler;
@@ -772,7 +772,7 @@ public <T extends TransportResponse> void sendRequest(
                             TransportRequestOptions options,
                             TransportResponseHandler<T> handler
                         ) {
-                            si.sendRequestDecorate(sender, connection, action, request, options, handler);
+                            si.sendRequestDecorate(sender, connection, action, request, options, handler, localNode.get());
                         }
                     };
                 }
@@ -1797,7 +1797,7 @@ public void onNodeStarted(DiscoveryNode localNode) {
         if (!SSLConfig.isSslOnlyMode() && !client && !disabled) {
             cr.initOnNodeStart();
         }
-        this.localNode = localNode;
+        this.localNode.set(localNode);
         final Set<ModuleInfo> securityModules = ReflectionHelper.getModulesLoaded();
         log.info("{} OpenSearch Security modules loaded so far: {}", securityModules.size(), securityModules);
     }
@@ -1877,14 +1877,6 @@ private static String handleKeyword(final String field) {
         return field;
     }
 
-    public static DiscoveryNode getLocalNode() {
-        return localNode;
-    }
-
-    public static void setLocalNode(DiscoveryNode node) {
-        localNode = node;
-    }
-
     public static class GuiceHolder implements LifecycleComponent {
 
         private static RepositoriesService repositoriesService;

diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java
@@ -130,7 +130,8 @@ public <T extends TransportResponse> void sendRequestDecorate(
         String action,
         TransportRequest request,
         TransportRequestOptions options,
-        TransportResponseHandler<T> handler
+        TransportResponseHandler<T> handler,
+        DiscoveryNode localNode
     ) {
         final Map<String, String> origHeaders0 = getThreadContext().getHeaders();
         final User user0 = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
@@ -146,8 +147,7 @@ public <T extends TransportResponse> void sendRequestDecorate(
         final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS);
 
         final boolean isDebugEnabled = log.isDebugEnabled();
-        final DiscoveryNode localNode = OpenSearchSecurityPlugin.getLocalNode();
-        boolean isSameNodeRequest = localNode != null && localNode.equals(connection.getNode());
+        final boolean isSameNodeRequest = localNode != null && localNode.equals(connection.getNode());
 
         try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) {
             final TransportResponseHandler<T> restoringHandler = new RestoringTransportResponseHandler<T>(handler, stashedContext);

diff --git a/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java b/src/test/java/org/opensearch/security/transport/SecurityInterceptorTests.java
@@ -147,11 +147,8 @@ public void testSendRequestDecorate() {
         DiscoveryNode otherNode = new DiscoveryNode("local-node", OpenSearchTestCase.buildNewFakeTransportAddress(), Version.CURRENT);
         Connection connection2 = transportService.getConnection(otherNode);
 
-        // setting localNode value explicitly
-        OpenSearchSecurityPlugin.setLocalNode(localNode);
-
         // isSameNodeRequest = true
-        securityInterceptor.sendRequestDecorate(sender, connection1, action, request, options, handler);
+        securityInterceptor.sendRequestDecorate(sender, connection1, action, request, options, handler, localNode);
         // from thread context inside sendRequestDecorate
         doAnswer(i -> {
             User transientUser = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
@@ -165,7 +162,7 @@ public void testSendRequestDecorate() {
         assertEquals(threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER), null);
 
         // isSameNodeRequest = false
-        securityInterceptor.sendRequestDecorate(sender, connection2, action, request, options, handler);
+        securityInterceptor.sendRequestDecorate(sender, connection2, action, request, options, handler, otherNode);
         // checking thread context inside sendRequestDecorate
         doAnswer(i -> {
             String serializedUserHeader = threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER);