Switch to ClusterManager terminology (#2062)

* Cluster manager inclusive checks on codebase Enforce usage of ClusterManager terminology in the codebase Include mechanism to disable checkstyle rule Cross cluster needs to have an additional role in the `node.roles` list, which I am guessing was backwards compatiable if the legacy versions of the role assignments were used. With this change cross cluster tests properly include this value during setup and the settings for these values are merged instead of being overridden. Signed-off-by: Peter Nied <[email protected]>
opensearch-project · Sep 12, 2022 · 7f992eb · 7f992eb
1 parent fd53d1d
commit 7f992eb
Show file tree

Hide file tree

Showing 17 changed files with 130 additions and 63 deletions.
diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
@@ -8,6 +8,7 @@ So you want to contribute code to this project? Excellent! We're glad you're her
   - [Using IntelliJ IDEA](#using-intellij-idea)
   - [Running integration tests](#running-integration-tests)
     - [Bulk test runs](#bulk-test-runs)
+    - [Checkstyle Violations](#checkstyle-violations)
   - [Submitting Changes](#submitting-changes)
   - [Backports](#backports)
 
@@ -146,6 +147,35 @@ Integration tests are automatically run on all pull requests for all supported v
 ### Bulk test runs
 To collect reliability data on test runs there is a manual GitHub action workflow called `Bulk Integration Test`.  The workflow is started for a branch on this project or in a fork by going to [GitHub action workflows](https://github.com/opensearch-project/security/actions/workflows/integration-tests.yml) and selecting `Run Workflow`.
 
+### Checkstyle Violations
+Checkstyle enforced several rules within this codebase.  Sometimes exceptions will be necessary for components that are set for deprecation but the new version is unavailable.  There are two formats of suppression that can be used when dealing with violations of this nature, one for disabling a single rule, or another for disabling all rules - its best to be as specific as possible.
+
+*Execute Checkstyle*
+```
+./gradlew checkstyleMain checkstyleTest 
+```
+
+*Example violation*
+```
+[ant:checkstyle] [ERROR] /local/home/security/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java:178: Usage should be switched to cluster manager [RegexpSingleline]
+```
+
+*Single Rule Suppression*
+```
+    // CS-SUPPRESS-SINGLE: RegexpSingleline See http://github/issues/1234
+    ...
+    Code that violates the rule
+    ...
+    // CS-ENFORCE-SINGLE
+```
+
+*Suppression All Checkstyle Rules*
+```
+  // CS-SUPRESS-ALL: Legacy code to be deleted in Z.Y.X see http://github/issues/1234
+  ...
+  // CS-ENFORCE-ALL
+```
+
 ## Submitting Changes
 
 See [CONTRIBUTING](CONTRIBUTING.md).

diff --git a/checkstyle/sun_checks.xml b/checkstyle/sun_checks.xml
@@ -201,7 +201,18 @@
     <property name="format" value="master"/>
     <property name="ignoreCase" value="true"/>
     <property name="message" value="Usage should be switched to cluster manager" />
-    <property name="severity" value="ignore"/>
+    <property name="severity" value="error"/>
+  </module>
+
+  <module name="SuppressWithPlainTextCommentFilter">
+    <property name="offCommentFormat" value="CS-SUPPRESS-ALL: .+"/> <!-- Require an explaination after surpressing -->
+    <property name="onCommentFormat" value="CS-ENFORCE-ALL"/>
+  </module>
+
+  <module name="SuppressWithPlainTextCommentFilter">
+    <property name="offCommentFormat" value="CS-SUPPRESS-SINGLE\: ([\w\|]+) .+"/> <!-- Require an explaination after surpressing -->
+    <property name="onCommentFormat" value="CS-ENFORCE-SINGLE"/>
+    <property name="checkFormat" value="$1"/>
   </module>
 
 </module>
diff --git a/src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java b/src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java
@@ -77,7 +77,7 @@ public void clusterChanged(ClusterChangedEvent event) {
             initialized = true;
         }
 
-        isLocalNodeElectedClusterManager = event.localNodeMaster()?Boolean.TRUE:Boolean.FALSE;
+        isLocalNodeElectedClusterManager = event.localNodeClusterManager()?Boolean.TRUE:Boolean.FALSE;
     }
 
     public Boolean getHas6xNodes() {

diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java
@@ -174,7 +174,9 @@ public boolean invoke(String action, ActionRequest request, final ActionListener
 
             //When we encounter a terms or sampler aggregation with masked fields activated we forcibly
             //need to switch off global ordinals because field masking can break ordering
+            // CS-SUPPRESS-SINGLE: RegexpSingleline Ignore term inside of url
             //https://www.elastic.co/guide/en/elasticsearch/reference/master/eager-global-ordinals.html#_avoiding_global_ordinal_loading
+            // CS-ENFORCE-SINGLE
             if (evaluatedDlsFlsConfig.hasFieldMasking()) {
 
                 if (searchRequest.source() != null && searchRequest.source().aggregations() != null) {

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java
@@ -11,6 +11,7 @@
 
 package org.opensearch.security.dlic.rest.api;
 
+// CS-SUPPRESS-SINGLE: RegexpSingleline https://github.com/opensearch-project/OpenSearch/issues/3663
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.Collections;
@@ -69,6 +70,7 @@
 import org.opensearch.threadpool.ThreadPool;
 
 import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
+// CS-ENFORCE-SINGLE
 
 public class MigrateApiAction extends AbstractApiAction {
     private static final List<Route> routes = addRoutesPrefix(Collections.singletonList(

diff --git a/src/main/java/org/opensearch/security/tools/SecurityAdmin.java b/src/main/java/org/opensearch/security/tools/SecurityAdmin.java
@@ -26,6 +26,7 @@
 
 package org.opensearch.security.tools;
 
+// CS-SUPPRESS-SINGLE: RegexpSingleline https://github.com/opensearch-project/OpenSearch/issues/3663
 import java.io.ByteArrayInputStream;
 import java.io.Console;
 import java.io.File;
@@ -139,6 +140,7 @@
 
 import static org.opensearch.common.xcontent.DeprecationHandler.THROW_UNSUPPORTED_OPERATION;
 import static org.opensearch.security.support.SecurityUtils.replaceEnvVars;
+// CS-ENFORCE-SINGLE
 
 @SuppressWarnings("deprecation")
 public class SecurityAdmin {

diff --git a/src/test/java/org/opensearch/security/RolesInjectorIntegTest.java b/src/test/java/org/opensearch/security/RolesInjectorIntegTest.java
@@ -45,6 +45,7 @@
 import org.opensearch.repositories.RepositoriesService;
 import org.opensearch.script.ScriptService;
 import org.opensearch.security.support.ConfigConstants;
+import org.opensearch.security.test.AbstractSecurityUnitTest;
 import org.opensearch.security.test.DynamicSecurityConfig;
 import org.opensearch.security.test.SingleClusterTest;
 import org.opensearch.threadpool.ThreadPool;
@@ -83,12 +84,9 @@ public void testRolesInject() throws Exception {
         Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().
                 health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
 
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", clusterInfo.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
                 .put("path.home", "./target")

diff --git a/src/test/java/org/opensearch/security/RolesValidationIntegTest.java b/src/test/java/org/opensearch/security/RolesValidationIntegTest.java
@@ -39,6 +39,7 @@
 import org.opensearch.repositories.RepositoriesService;
 import org.opensearch.script.ScriptService;
 import org.opensearch.security.support.ConfigConstants;
+import org.opensearch.security.test.AbstractSecurityUnitTest;
 import org.opensearch.security.test.DynamicSecurityConfig;
 import org.opensearch.security.test.SingleClusterTest;
 import org.opensearch.threadpool.ThreadPool;
@@ -74,12 +75,9 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
     public void testRolesValidation() throws Exception {
         setup(Settings.EMPTY, new DynamicSecurityConfig().setSecurityRoles("roles.yml"), Settings.EMPTY);
 
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", clusterInfo.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
                 .put("path.home", "./target")

diff --git a/src/test/java/org/opensearch/security/SlowIntegrationTests.java b/src/test/java/org/opensearch/security/SlowIntegrationTests.java
@@ -42,6 +42,7 @@
 import org.opensearch.node.PluginAwareNode;
 import org.opensearch.security.ssl.util.SSLConfigConstants;
 import org.opensearch.security.support.ConfigConstants;
+import org.opensearch.security.test.AbstractSecurityUnitTest;
 import org.opensearch.security.test.SingleClusterTest;
 import org.opensearch.security.test.helper.cluster.ClusterConfiguration;
 import org.opensearch.security.test.helper.file.FileHelper;
@@ -70,12 +71,9 @@ public void testNodeClientAllowedWithServerCertificate() throws Exception {
         Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
 
 
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", clusterInfo.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
                 .put("path.home", "./target")
@@ -100,12 +98,9 @@ public void testNodeClientDisallowedWithNonServerCertificate() throws Exception
         Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
 
 
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", clusterInfo.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
                 .put("path.home", "./target")
@@ -134,12 +129,9 @@ public void testNodeClientDisallowedWithNonServerCertificate2() throws Exception
         Assert.assertEquals(clusterInfo.numNodes, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getNumberOfNodes());
         Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
 
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", clusterInfo.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
                 .put("path.home", "./target")

diff --git a/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java b/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java
@@ -37,6 +37,7 @@
 import org.opensearch.repositories.RepositoriesService;
 import org.opensearch.script.ScriptService;
 import org.opensearch.security.support.ConfigConstants;
+import org.opensearch.security.test.AbstractSecurityUnitTest;
 import org.opensearch.security.test.DynamicSecurityConfig;
 import org.opensearch.security.test.SingleClusterTest;
 import org.opensearch.threadpool.ThreadPool;
@@ -72,12 +73,9 @@ public void testSecurityUserInjection() throws Exception {
                 .put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, true)
                 .build();
         setup(clusterNodeSettings, new DynamicSecurityConfig().setSecurityRolesMapping("roles_transport_inject_user.yml"), Settings.EMPTY);
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", clusterInfo.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
                 .put("path.home", "./target")
@@ -129,12 +127,9 @@ public void testSecurityUserInjectionWithConfigDisabled() throws Exception {
                 .put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, false)
                 .build();
         setup(clusterNodeSettings, new DynamicSecurityConfig().setSecurityRolesMapping("roles_transport_inject_user.yml"), Settings.EMPTY);
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", clusterInfo.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
                 .put("path.home", "./target")

diff --git a/src/test/java/org/opensearch/security/ccstest/CrossClusterSearchTests.java b/src/test/java/org/opensearch/security/ccstest/CrossClusterSearchTests.java
@@ -84,7 +84,9 @@ public ClusterTransportClientSettings() {
         }
 
         public ClusterTransportClientSettings(Settings clusterSettings, Settings transportSettings) {
-            super(clusterSettings, transportSettings);
+            super(Settings.builder()
+                .put(clusterSettings)
+                .putList("node.roles", "remote_cluster_client").build(), transportSettings);
         }
 
         public Settings clusterSettings() {
@@ -1001,12 +1003,10 @@ public void testCcsWithRoleInjection() throws Exception {
                     .source("{\"cluster\": \""+cl2Info.clustername+"\"}", XContentType.JSON)).actionGet();
         }
 
-        final Settings tcSettings = Settings.builder()
+        final Settings.Builder clusterClientSettings = Settings.builder().putList("node.roles", "remote_cluster_client");
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(clusterClientSettings, false, false) 
                 .put(minimumSecuritySettings(Settings.EMPTY).get(0))
                 .put("cluster.name", cl1Info.clustername)
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + cl1Info.clustername + "/cert/data")
                 .put("path.logs", "./target/data/" + cl1Info.clustername + "/cert/logs")
                 .put("path.home", "./target")

diff --git a/src/test/java/org/opensearch/security/dlic/dlsfls/CCReplicationTest.java b/src/test/java/org/opensearch/security/dlic/dlsfls/CCReplicationTest.java
@@ -11,6 +11,7 @@
 
 package org.opensearch.security.dlic.dlsfls;
 
+// CS-SUPPRESS-SINGLE: RegexpSingleline https://github.com/opensearch-project/OpenSearch/issues/3663
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -59,12 +60,14 @@
 import org.opensearch.script.ScriptService;
 import org.opensearch.security.OpenSearchSecurityPlugin;
 import org.opensearch.security.support.ConfigConstants;
+import org.opensearch.security.test.AbstractSecurityUnitTest;
 import org.opensearch.security.test.DynamicSecurityConfig;
 import org.opensearch.tasks.Task;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.Netty4Plugin;
 import org.opensearch.transport.TransportService;
 import org.opensearch.watcher.ResourceWatcherService;
+// CS-ENFORCE-SINGLE
 
 public class CCReplicationTest extends AbstractDlsFlsTest {
     public static class MockReplicationPlugin extends Plugin implements ActionPlugin {
@@ -180,14 +183,11 @@ public void testReplication() throws Exception {
         Assert.assertEquals(clusterInfo.numNodes, clusterHelper.nodeClient().admin().cluster().health(
             new ClusterHealthRequest().waitForGreenStatus()).actionGet().getNumberOfNodes());
         Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().
-            health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
+        health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
 
-        final Settings tcSettings = Settings.builder()
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
             .put(minimumSecuritySettings(Settings.EMPTY).get(0))
             .put("cluster.name", clusterInfo.clustername)
-            .put("node.data", false)
-            .put("node.master", false)
-            .put("node.ingest", false)
             .put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
             .put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
             .put("path.home", "./target")

diff --git a/src/test/java/org/opensearch/security/dlic/dlsfls/DlsFlsCrossClusterSearchTest.java b/src/test/java/org/opensearch/security/dlic/dlsfls/DlsFlsCrossClusterSearchTest.java
@@ -47,7 +47,7 @@ private void setupCcs(String remoteRoles) throws Exception {
 
         System.setProperty("security.display_lic_none","true");
 
-        cl2Info = cl2.startCluster(minimumSecuritySettings(Settings.EMPTY), ClusterConfiguration.DEFAULT);
+        cl2Info = cl2.startCluster(minimumSecuritySettings(Settings.builder().putList("node.roles", "remote_cluster_client").build()), ClusterConfiguration.DEFAULT);
         initialize(cl2, cl2Info, new DynamicSecurityConfig().setSecurityRoles(remoteRoles));
         System.out.println("### cl2 complete ###");
 
@@ -66,7 +66,8 @@ public void tearDown() throws Exception {
 
     private Settings crossClusterNodeSettings(ClusterInfo remote) {
         Settings.Builder builder = Settings.builder()
-                .putList("cluster.remote.cross_cluster_two.seeds", remote.nodeHost+":"+remote.nodePort);
+                .putList("cluster.remote.cross_cluster_two.seeds", remote.nodeHost+":"+remote.nodePort)
+                .putList("node.roles", "remote_cluster_client");
         return builder.build();
     }
 

diff --git a/src/test/java/org/opensearch/security/ssl/OpenSSLTest.java b/src/test/java/org/opensearch/security/ssl/OpenSSLTest.java
@@ -40,6 +40,7 @@
 import org.opensearch.security.OpenSearchSecurityPlugin;
 import org.opensearch.security.ssl.util.SSLConfigConstants;
 import org.opensearch.security.support.ConfigConstants;
+import org.opensearch.security.test.AbstractSecurityUnitTest;
 import org.opensearch.security.test.helper.file.FileHelper;
 import org.opensearch.security.test.helper.rest.RestHelper;
 import org.opensearch.transport.Netty4Plugin;
@@ -206,11 +207,9 @@ public void testNodeClientSSLwithOpenSslTLSv13() throws Exception {
 
         RestHelper rh = nonSslRestHelper();
 
-        final Settings tcSettings = Settings.builder().put("cluster.name", clusterInfo.clustername).put("path.home", "/tmp")
+        final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false) 
+                .put("cluster.name", clusterInfo.clustername).put("path.home", "/tmp")
                 .put("node.name", "client_node_" + new Random().nextInt())
-                .put("node.data", false)
-                .put("node.master", false)
-                .put("node.ingest", false)
                 .put("path.data", "./target/data/" + clusterInfo.clustername + "/ssl/data")
                 .put("path.logs", "./target/data/" + clusterInfo.clustername + "/ssl/logs")
                 .put("path.home", "./target")