Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate setting 'reindex.remote.whitelist' and introduce the alternative setting 'reindex.remote.allowlist' #2221

Merged
merged 11 commits into from
Mar 8, 2022
2 changes: 1 addition & 1 deletion client/rest-high-level/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ check.dependsOn(asyncIntegTest)
testClusters.all {
testDistribution = 'ARCHIVE'
systemProperty 'opensearch.scripting.update.ctx_in_params', 'false'
setting 'reindex.remote.whitelist', '[ "[::1]:*", "127.0.0.1:*" ]'
setting 'reindex.remote.allowlist', '[ "[::1]:*", "127.0.0.1:*" ]'

extraConfigFile 'roles.yml', file('roles.yml')
user username: System.getProperty('tests.rest.cluster.username', 'test_user'),
Expand Down
2 changes: 1 addition & 1 deletion modules/reindex/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ testClusters.all {
module ':modules:parent-join'
module ':modules:lang-painless'
// Whitelist reindexing from the local node so we can test reindex-from-remote.
setting 'reindex.remote.whitelist', '127.0.0.1:*'
setting 'reindex.remote.allowlist', '127.0.0.1:*'
}

test {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ public Collection<Object> createComponents(
public List<Setting<?>> getSettings() {
final List<Setting<?>> settings = new ArrayList<>();
settings.add(TransportReindexAction.REMOTE_CLUSTER_WHITELIST);
settings.add(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST);
settings.addAll(ReindexSslConfig.getSettings());
return settings;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class ReindexValidator {
IndexNameExpressionResolver resolver,
AutoCreateIndex autoCreateIndex
) {
this.remoteWhitelist = buildRemoteWhitelist(TransportReindexAction.REMOTE_CLUSTER_WHITELIST.get(settings));
this.remoteWhitelist = buildRemoteWhitelist(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.get(settings));
this.clusterService = clusterService;
this.resolver = resolver;
this.autoCreateIndex = autoCreateIndex;
Expand Down Expand Up @@ -101,7 +101,7 @@ static void checkRemoteWhitelist(CharacterRunAutomaton whitelist, RemoteInfo rem
if (whitelist.run(check)) {
return;
}
String whiteListKey = TransportReindexAction.REMOTE_CLUSTER_WHITELIST.getKey();
String whiteListKey = TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.getKey();
throw new IllegalArgumentException('[' + check + "] not whitelisted in " + whiteListKey);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this exception message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @andrross, renaming the exception message is covered in PR #2178 , so I didn't touch it. 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since PR #2178 is merged, the "whitelisted" in the exception messages have changed to "allowlisted".

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,19 @@
import static java.util.Collections.emptyList;

public class TransportReindexAction extends HandledTransportAction<ReindexRequest, BulkByScrollResponse> {
public static final Setting<List<String>> REMOTE_CLUSTER_WHITELIST = Setting.listSetting(
static final Setting<List<String>> REMOTE_CLUSTER_WHITELIST = Setting.listSetting(
"reindex.remote.whitelist",
emptyList(),
Function.identity(),
Property.NodeScope,
Property.Deprecated
);
// The setting below is going to replace the above.
// To keep backwards compatibility, the old usage is remained, and it's also used as the fallback for the new usage.
public static final Setting<List<String>> REMOTE_CLUSTER_ALLOWLIST = Setting.listSetting(
"reindex.remote.allowlist",
REMOTE_CLUSTER_WHITELIST,
Function.identity(),
Property.NodeScope
);
public static Optional<RemoteReindexExtension> remoteExtension = Optional.empty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public void testUnwhitelistedRemote() {
IllegalArgumentException.class,
() -> checkRemoteWhitelist(buildRemoteWhitelist(whitelist), newRemoteInfo("not in list", port))
);
assertEquals("[not in list:" + port + "] not whitelisted in reindex.remote.whitelist", e.getMessage());
assertEquals("[not in list:" + port + "] not whitelisted in reindex.remote.allowlist", e.getMessage());
}

public void testRejectMatchAll() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ protected boolean addMockHttpTransport() {
protected Settings nodeSettings() {
Settings.Builder settings = Settings.builder().put(super.nodeSettings());
// Whitelist reindexing from the http host we're going to use
settings.put(TransportReindexAction.REMOTE_CLUSTER_WHITELIST.getKey(), "127.0.0.1:*");
settings.put(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.getKey(), "127.0.0.1:*");
settings.put(NetworkModule.HTTP_TYPE_KEY, Netty4Plugin.NETTY_HTTP_TRANSPORT_NAME);
return settings.build();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.index.reindex;

import org.junit.Before;
import org.opensearch.common.settings.Setting;
import org.opensearch.common.settings.Settings;
import org.opensearch.test.OpenSearchTestCase;

import java.util.Arrays;
import java.util.List;

/**
* A unit test to validate the former name of the setting 'reindex.remote.allowlist' still take effect,
* after it is deprecated, so that the backwards compatibility is maintained.
* The test can be removed along with removing support of the deprecated setting.
*/
public class ReindexRenamedSettingTests extends OpenSearchTestCase {
ReindexPlugin plugin;

@Before
public void setup() {
this.plugin = new ReindexPlugin();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but I think you can replace all this with just:

private final ReindexPlugin plugin = new ReindexPlugin();

There's generally no need for a @Before method unless you need more complex logic than field assignment. (One note about JUnit that often trips people up...the test framework creates a brand new instance of the test class before each individual test run. That mean no instance state is kept between test runs, which is why the simple inline assignment functions the same as assigning fields in a @Before method.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for sharing this knowledge to me! 👍👍 I totally had no idea with it. 😲 I will make the change.

Copy link
Member

@andrross andrross Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JUnit5 allows you to specify the test instance lifecycle, though the create-new-instance-for-each-test-method behavior is the default and matches the behavior of all previous versions of JUnit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for telling me the background theory! Hope more people notice it. 😄 I see, since the default lifecycle is "per method", so any class variable will be initialized before calling any method.
(Though in this file, the variable only used in one method 😅)


/**
* Validate the both settings are known and supported.
*/
public void testReindexSettingsExist() {
List<Setting<?>> settings = plugin.getSettings();
assertTrue(
"Both 'reindex.remote.allowlist' and its predecessor should be supported settings of Reindex plugin",
settings.containsAll(
Arrays.asList(TransportReindexAction.REMOTE_CLUSTER_WHITELIST, TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST)
)
);
}

/**
* Validate the default value of the both settings is the same.
*/
public void testSettingFallback() {
assertEquals(
TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.get(Settings.EMPTY),
TransportReindexAction.REMOTE_CLUSTER_WHITELIST.get(Settings.EMPTY)
);
}

/**
* Validate the new setting can be configured correctly, and it doesn't impact the old setting.
*/
public void testSettingGetValue() {
Settings settings = Settings.builder().put("reindex.remote.allowlist", "127.0.0.1:*").build();
assertEquals(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.get(settings), Arrays.asList("127.0.0.1:*"));
assertEquals(
TransportReindexAction.REMOTE_CLUSTER_WHITELIST.get(settings),
TransportReindexAction.REMOTE_CLUSTER_WHITELIST.getDefault(Settings.EMPTY)
);
}

/**
* Validate the value of the old setting will be applied to the new setting, if the new setting is not configured.
*/
public void testSettingGetValueWithFallback() {
Settings settings = Settings.builder().put("reindex.remote.whitelist", "127.0.0.1:*").build();
assertEquals(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.get(settings), Arrays.asList("127.0.0.1:*"));
assertSettingDeprecationsAndWarnings(new Setting<?>[] { TransportReindexAction.REMOTE_CLUSTER_WHITELIST });
}

/**
* Validate the value of the old setting will be ignored, if the new setting is configured.
*/
public void testSettingGetValueWhenBothAreConfigured() {
Settings settings = Settings.builder()
.put("reindex.remote.allowlist", "127.0.0.1:*")
.put("reindex.remote.whitelist", "[::1]:*, 127.0.0.1:*")
.build();
assertEquals(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.get(settings), Arrays.asList("127.0.0.1:*"));
assertEquals(TransportReindexAction.REMOTE_CLUSTER_WHITELIST.get(settings), Arrays.asList("[::1]:*", "127.0.0.1:*"));
assertSettingDeprecationsAndWarnings(new Setting<?>[] { TransportReindexAction.REMOTE_CLUSTER_WHITELIST });
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ protected boolean addMockHttpTransport() {
final Settings nodeSettings() {
return Settings.builder()
// whitelist reindexing from the HTTP host we're going to use
.put(TransportReindexAction.REMOTE_CLUSTER_WHITELIST.getKey(), "127.0.0.1:*")
.put(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.getKey(), "127.0.0.1:*")
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@
---
"unwhitelisted remote host fails":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this test description be changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thank you. This one is missed in PR #2178, I changed it in this PR.

- do:
catch: /\[badremote:9200\] not whitelisted in reindex.remote.whitelist/
catch: /\[badremote:9200\] not whitelisted in reindex.remote.allowlist/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be updated if you change the exception message, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. (Since PR #2178 is merged, the "whitelisted" in the exception messages have changed to "allowlisted") 😁

reindex:
body:
source:
Expand Down