Skip to content

Commit 7f992eb

Browse files
peterniedpeternied
authored
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 <petern@amazon.com>
1 parent fd53d1d commit 7f992eb

17 files changed

Lines changed: 130 additions & 63 deletions

‎DEVELOPER_GUIDE.md‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ So you want to contribute code to this project? Excellent! We're glad you're her
88
- [Using IntelliJ IDEA](#using-intellij-idea)
99
- [Running integration tests](#running-integration-tests)
1010
- [Bulk test runs](#bulk-test-runs)
11+
- [Checkstyle Violations](#checkstyle-violations)
1112
- [Submitting Changes](#submitting-changes)
1213
- [Backports](#backports)
1314

@@ -146,6 +147,35 @@ Integration tests are automatically run on all pull requests for all supported v
146147
### Bulk test runs
147148
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`.
148149

150+
### Checkstyle Violations
151+
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.
152+
153+
*Execute Checkstyle*
154+
```
155+
./gradlew checkstyleMain checkstyleTest
156+
```
157+
158+
*Example violation*
159+
```
160+
[ant:checkstyle] [ERROR] /local/home/security/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java:178: Usage should be switched to cluster manager [RegexpSingleline]
161+
```
162+
163+
*Single Rule Suppression*
164+
```
165+
// CS-SUPPRESS-SINGLE: RegexpSingleline See http://github/issues/1234
166+
...
167+
Code that violates the rule
168+
...
169+
// CS-ENFORCE-SINGLE
170+
```
171+
172+
*Suppression All Checkstyle Rules*
173+
```
174+
// CS-SUPRESS-ALL: Legacy code to be deleted in Z.Y.X see http://github/issues/1234
175+
...
176+
// CS-ENFORCE-ALL
177+
```
178+
149179
## Submitting Changes
150180

151181
See [CONTRIBUTING](CONTRIBUTING.md).

‎checkstyle/sun_checks.xml‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,18 @@
201201
<property name="format" value="master"/>
202202
<property name="ignoreCase" value="true"/>
203203
<property name="message" value="Usage should be switched to cluster manager" />
204-
<property name="severity" value="ignore"/>
204+
<property name="severity" value="error"/>
205+
</module>
206+
207+
<module name="SuppressWithPlainTextCommentFilter">
208+
<property name="offCommentFormat" value="CS-SUPPRESS-ALL: .+"/> <!-- Require an explaination after surpressing -->
209+
<property name="onCommentFormat" value="CS-ENFORCE-ALL"/>
210+
</module>
211+
212+
<module name="SuppressWithPlainTextCommentFilter">
213+
<property name="offCommentFormat" value="CS-SUPPRESS-SINGLE\: ([\w\|]+) .+"/> <!-- Require an explaination after surpressing -->
214+
<property name="onCommentFormat" value="CS-ENFORCE-SINGLE"/>
215+
<property name="checkFormat" value="$1"/>
205216
</module>
206217

207218
</module>

‎src/main/java/org/opensearch/security/configuration/ClusterInfoHolder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public void clusterChanged(ClusterChangedEvent event) {
7777
initialized = true;
7878
}
7979

80-
isLocalNodeElectedClusterManager = event.localNodeMaster()?Boolean.TRUE:Boolean.FALSE;
80+
isLocalNodeElectedClusterManager = event.localNodeClusterManager()?Boolean.TRUE:Boolean.FALSE;
8181
}
8282

8383
public Boolean getHas6xNodes() {

‎src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,9 @@ public boolean invoke(String action, ActionRequest request, final ActionListener
174174

175175
//When we encounter a terms or sampler aggregation with masked fields activated we forcibly
176176
//need to switch off global ordinals because field masking can break ordering
177+
// CS-SUPPRESS-SINGLE: RegexpSingleline Ignore term inside of url
177178
//https://www.elastic.co/guide/en/elasticsearch/reference/master/eager-global-ordinals.html#_avoiding_global_ordinal_loading
179+
// CS-ENFORCE-SINGLE
178180
if (evaluatedDlsFlsConfig.hasFieldMasking()) {
179181

180182
if (searchRequest.source() != null && searchRequest.source().aggregations() != null) {

‎src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
package org.opensearch.security.dlic.rest.api;
1313

14+
// CS-SUPPRESS-SINGLE: RegexpSingleline https://github.com/opensearch-project/OpenSearch/issues/3663
1415
import java.io.IOException;
1516
import java.nio.file.Path;
1617
import java.util.Collections;
@@ -69,6 +70,7 @@
6970
import org.opensearch.threadpool.ThreadPool;
7071

7172
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
73+
// CS-ENFORCE-SINGLE
7274

7375
public class MigrateApiAction extends AbstractApiAction {
7476
private static final List<Route> routes = addRoutesPrefix(Collections.singletonList(

‎src/main/java/org/opensearch/security/tools/SecurityAdmin.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
package org.opensearch.security.tools;
2828

29+
// CS-SUPPRESS-SINGLE: RegexpSingleline https://github.com/opensearch-project/OpenSearch/issues/3663
2930
import java.io.ByteArrayInputStream;
3031
import java.io.Console;
3132
import java.io.File;
@@ -139,6 +140,7 @@
139140

140141
import static org.opensearch.common.xcontent.DeprecationHandler.THROW_UNSUPPORTED_OPERATION;
141142
import static org.opensearch.security.support.SecurityUtils.replaceEnvVars;
143+
// CS-ENFORCE-SINGLE
142144

143145
@SuppressWarnings("deprecation")
144146
public class SecurityAdmin {

‎src/test/java/org/opensearch/security/RolesInjectorIntegTest.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import org.opensearch.repositories.RepositoriesService;
4646
import org.opensearch.script.ScriptService;
4747
import org.opensearch.security.support.ConfigConstants;
48+
import org.opensearch.security.test.AbstractSecurityUnitTest;
4849
import org.opensearch.security.test.DynamicSecurityConfig;
4950
import org.opensearch.security.test.SingleClusterTest;
5051
import org.opensearch.threadpool.ThreadPool;
@@ -83,12 +84,9 @@ public void testRolesInject() throws Exception {
8384
Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().
8485
health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
8586

86-
final Settings tcSettings = Settings.builder()
87+
final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false)
8788
.put(minimumSecuritySettings(Settings.EMPTY).get(0))
8889
.put("cluster.name", clusterInfo.clustername)
89-
.put("node.data", false)
90-
.put("node.master", false)
91-
.put("node.ingest", false)
9290
.put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
9391
.put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
9492
.put("path.home", "./target")

‎src/test/java/org/opensearch/security/RolesValidationIntegTest.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.opensearch.repositories.RepositoriesService;
4040
import org.opensearch.script.ScriptService;
4141
import org.opensearch.security.support.ConfigConstants;
42+
import org.opensearch.security.test.AbstractSecurityUnitTest;
4243
import org.opensearch.security.test.DynamicSecurityConfig;
4344
import org.opensearch.security.test.SingleClusterTest;
4445
import org.opensearch.threadpool.ThreadPool;
@@ -74,12 +75,9 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
7475
public void testRolesValidation() throws Exception {
7576
setup(Settings.EMPTY, new DynamicSecurityConfig().setSecurityRoles("roles.yml"), Settings.EMPTY);
7677

77-
final Settings tcSettings = Settings.builder()
78+
final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false)
7879
.put(minimumSecuritySettings(Settings.EMPTY).get(0))
7980
.put("cluster.name", clusterInfo.clustername)
80-
.put("node.data", false)
81-
.put("node.master", false)
82-
.put("node.ingest", false)
8381
.put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
8482
.put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
8583
.put("path.home", "./target")

‎src/test/java/org/opensearch/security/SlowIntegrationTests.java‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.opensearch.node.PluginAwareNode;
4343
import org.opensearch.security.ssl.util.SSLConfigConstants;
4444
import org.opensearch.security.support.ConfigConstants;
45+
import org.opensearch.security.test.AbstractSecurityUnitTest;
4546
import org.opensearch.security.test.SingleClusterTest;
4647
import org.opensearch.security.test.helper.cluster.ClusterConfiguration;
4748
import org.opensearch.security.test.helper.file.FileHelper;
@@ -70,12 +71,9 @@ public void testNodeClientAllowedWithServerCertificate() throws Exception {
7071
Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
7172

7273

73-
final Settings tcSettings = Settings.builder()
74+
final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false)
7475
.put(minimumSecuritySettings(Settings.EMPTY).get(0))
7576
.put("cluster.name", clusterInfo.clustername)
76-
.put("node.data", false)
77-
.put("node.master", false)
78-
.put("node.ingest", false)
7977
.put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
8078
.put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
8179
.put("path.home", "./target")
@@ -100,12 +98,9 @@ public void testNodeClientDisallowedWithNonServerCertificate() throws Exception
10098
Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
10199

102100

103-
final Settings tcSettings = Settings.builder()
101+
final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false)
104102
.put(minimumSecuritySettings(Settings.EMPTY).get(0))
105103
.put("cluster.name", clusterInfo.clustername)
106-
.put("node.data", false)
107-
.put("node.master", false)
108-
.put("node.ingest", false)
109104
.put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
110105
.put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
111106
.put("path.home", "./target")
@@ -134,12 +129,9 @@ public void testNodeClientDisallowedWithNonServerCertificate2() throws Exception
134129
Assert.assertEquals(clusterInfo.numNodes, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getNumberOfNodes());
135130
Assert.assertEquals(ClusterHealthStatus.GREEN, clusterHelper.nodeClient().admin().cluster().health(new ClusterHealthRequest().waitForGreenStatus()).actionGet().getStatus());
136131

137-
final Settings tcSettings = Settings.builder()
132+
final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false)
138133
.put(minimumSecuritySettings(Settings.EMPTY).get(0))
139134
.put("cluster.name", clusterInfo.clustername)
140-
.put("node.data", false)
141-
.put("node.master", false)
142-
.put("node.ingest", false)
143135
.put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
144136
.put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
145137
.put("path.home", "./target")

‎src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.opensearch.repositories.RepositoriesService;
3838
import org.opensearch.script.ScriptService;
3939
import org.opensearch.security.support.ConfigConstants;
40+
import org.opensearch.security.test.AbstractSecurityUnitTest;
4041
import org.opensearch.security.test.DynamicSecurityConfig;
4142
import org.opensearch.security.test.SingleClusterTest;
4243
import org.opensearch.threadpool.ThreadPool;
@@ -72,12 +73,9 @@ public void testSecurityUserInjection() throws Exception {
7273
.put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, true)
7374
.build();
7475
setup(clusterNodeSettings, new DynamicSecurityConfig().setSecurityRolesMapping("roles_transport_inject_user.yml"), Settings.EMPTY);
75-
final Settings tcSettings = Settings.builder()
76+
final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false)
7677
.put(minimumSecuritySettings(Settings.EMPTY).get(0))
7778
.put("cluster.name", clusterInfo.clustername)
78-
.put("node.data", false)
79-
.put("node.master", false)
80-
.put("node.ingest", false)
8179
.put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
8280
.put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
8381
.put("path.home", "./target")
@@ -129,12 +127,9 @@ public void testSecurityUserInjectionWithConfigDisabled() throws Exception {
129127
.put(ConfigConstants.SECURITY_UNSUPPORTED_INJECT_USER_ENABLED, false)
130128
.build();
131129
setup(clusterNodeSettings, new DynamicSecurityConfig().setSecurityRolesMapping("roles_transport_inject_user.yml"), Settings.EMPTY);
132-
final Settings tcSettings = Settings.builder()
130+
final Settings tcSettings = AbstractSecurityUnitTest.nodeRolesSettings(Settings.builder(), false, false)
133131
.put(minimumSecuritySettings(Settings.EMPTY).get(0))
134132
.put("cluster.name", clusterInfo.clustername)
135-
.put("node.data", false)
136-
.put("node.master", false)
137-
.put("node.ingest", false)
138133
.put("path.data", "./target/data/" + clusterInfo.clustername + "/cert/data")
139134
.put("path.logs", "./target/data/" + clusterInfo.clustername + "/cert/logs")
140135
.put("path.home", "./target")

0 commit comments

Comments
 (0)