From 63c75d1b1d188a4bc3917169001aa8711ad702ef Mon Sep 17 00:00:00 2001 From: Tianli Feng Date: Tue, 8 Mar 2022 08:35:36 -0800 Subject: [PATCH] Deprecate setting 'reindex.remote.whitelist' and introduce the alternative setting 'reindex.remote.allowlist' (#2221) * Add setting reindex.remote.allowlist, and deprecate setting reindex.remote.whitelist Signed-off-by: Tianli Feng * Add unit test for renaming the setting reindex.remote.allowlist Signed-off-by: Tianli Feng * Remove system.out.println() Signed-off-by: Tianli Feng * Adjust format by spotlessApply task Signed-off-by: Tianli Feng * Replace REMOTE_CLUSTER_WHITELIST with REMOTE_CLUSTER_ALLOWLIST Signed-off-by: Tianli Feng * Add a unit test to test final setting value when both settings have got a value Signed-off-by: Tianli Feng * Rename the unit test class name Signed-off-by: Tianli Feng * Remove the Access modifiers public from the constant REMOTE_CLUSTER_WHITELIST Signed-off-by: Tianli Feng * Initialize ReindexPlugin without using the @Before method Signed-off-by: Tianli Feng * Rename 'unwhitelisted' to 'unallowlisted' in a yml file used for REST api testing. Signed-off-by: Tianli Feng --- client/rest-high-level/build.gradle | 2 +- modules/reindex/build.gradle | 2 +- .../index/reindex/ReindexPlugin.java | 1 + .../index/reindex/ReindexValidator.java | 4 +- .../index/reindex/TransportReindexAction.java | 11 ++- .../ReindexFromRemoteWhitelistTests.java | 2 +- .../ReindexFromRemoteWithAuthTests.java | 2 +- .../reindex/ReindexRenamedSettingTests.java | 83 +++++++++++++++++++ .../opensearch/index/reindex/RetryTests.java | 2 +- .../test/reindex/20_validation.yml | 4 +- 10 files changed, 103 insertions(+), 10 deletions(-) create mode 100644 modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexRenamedSettingTests.java diff --git a/client/rest-high-level/build.gradle b/client/rest-high-level/build.gradle index 4144186ba5f70..07147ce81b72e 100644 --- a/client/rest-high-level/build.gradle +++ b/client/rest-high-level/build.gradle @@ -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'), diff --git a/modules/reindex/build.gradle b/modules/reindex/build.gradle index 935fe468fdbd0..37526a924da73 100644 --- a/modules/reindex/build.gradle +++ b/modules/reindex/build.gradle @@ -50,7 +50,7 @@ testClusters.all { module ':modules:parent-join' module ':modules:lang-painless' // Allowlist 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 { diff --git a/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexPlugin.java b/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexPlugin.java index 04619efb43c6c..865ae26f6f54d 100644 --- a/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexPlugin.java +++ b/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexPlugin.java @@ -133,6 +133,7 @@ public Collection createComponents( public List> getSettings() { final List> settings = new ArrayList<>(); settings.add(TransportReindexAction.REMOTE_CLUSTER_WHITELIST); + settings.add(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST); settings.addAll(ReindexSslConfig.getSettings()); return settings; } diff --git a/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexValidator.java b/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexValidator.java index d4a2ba08409e6..71c3aad8713e1 100644 --- a/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexValidator.java +++ b/modules/reindex/src/main/java/org/opensearch/index/reindex/ReindexValidator.java @@ -70,7 +70,7 @@ class ReindexValidator { IndexNameExpressionResolver resolver, AutoCreateIndex autoCreateIndex ) { - this.remoteAllowlist = buildRemoteAllowlist(TransportReindexAction.REMOTE_CLUSTER_WHITELIST.get(settings)); + this.remoteAllowlist = buildRemoteAllowlist(TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.get(settings)); this.clusterService = clusterService; this.resolver = resolver; this.autoCreateIndex = autoCreateIndex; @@ -101,7 +101,7 @@ static void checkRemoteAllowlist(CharacterRunAutomaton allowlist, RemoteInfo rem if (allowlist.run(check)) { return; } - String allowListKey = TransportReindexAction.REMOTE_CLUSTER_WHITELIST.getKey(); + String allowListKey = TransportReindexAction.REMOTE_CLUSTER_ALLOWLIST.getKey(); throw new IllegalArgumentException('[' + check + "] not allowlisted in " + allowListKey); } diff --git a/modules/reindex/src/main/java/org/opensearch/index/reindex/TransportReindexAction.java b/modules/reindex/src/main/java/org/opensearch/index/reindex/TransportReindexAction.java index a24c2b002b759..c84d103a2ef6f 100644 --- a/modules/reindex/src/main/java/org/opensearch/index/reindex/TransportReindexAction.java +++ b/modules/reindex/src/main/java/org/opensearch/index/reindex/TransportReindexAction.java @@ -56,10 +56,19 @@ import static java.util.Collections.emptyList; public class TransportReindexAction extends HandledTransportAction { - public static final Setting> REMOTE_CLUSTER_WHITELIST = Setting.listSetting( + static final Setting> 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> REMOTE_CLUSTER_ALLOWLIST = Setting.listSetting( + "reindex.remote.allowlist", + REMOTE_CLUSTER_WHITELIST, + Function.identity(), Property.NodeScope ); public static Optional remoteExtension = Optional.empty(); diff --git a/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWhitelistTests.java b/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWhitelistTests.java index df2d9894e64bb..8012b67253cb6 100644 --- a/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWhitelistTests.java +++ b/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWhitelistTests.java @@ -131,7 +131,7 @@ public void testUnwhitelistedRemote() { IllegalArgumentException.class, () -> checkRemoteAllowlist(buildRemoteAllowlist(allowlist), newRemoteInfo("not in list", port)) ); - assertEquals("[not in list:" + port + "] not allowlisted in reindex.remote.whitelist", e.getMessage()); + assertEquals("[not in list:" + port + "] not allowlisted in reindex.remote.allowlist", e.getMessage()); } public void testRejectMatchAll() { diff --git a/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWithAuthTests.java b/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWithAuthTests.java index e78715d904574..8ce850a936557 100644 --- a/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWithAuthTests.java +++ b/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWithAuthTests.java @@ -99,7 +99,7 @@ protected boolean addMockHttpTransport() { protected Settings nodeSettings() { Settings.Builder settings = Settings.builder().put(super.nodeSettings()); // Allowlist 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(); } diff --git a/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexRenamedSettingTests.java b/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexRenamedSettingTests.java new file mode 100644 index 0000000000000..8ff84223d371e --- /dev/null +++ b/modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexRenamedSettingTests.java @@ -0,0 +1,83 @@ +/* + * 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.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 { + private final ReindexPlugin plugin = new ReindexPlugin(); + + /** + * Validate the both settings are known and supported. + */ + public void testReindexSettingsExist() { + List> 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 }); + } + +} diff --git a/modules/reindex/src/test/java/org/opensearch/index/reindex/RetryTests.java b/modules/reindex/src/test/java/org/opensearch/index/reindex/RetryTests.java index 96b1b5d3d2e65..124670dba9510 100644 --- a/modules/reindex/src/test/java/org/opensearch/index/reindex/RetryTests.java +++ b/modules/reindex/src/test/java/org/opensearch/index/reindex/RetryTests.java @@ -103,7 +103,7 @@ protected boolean addMockHttpTransport() { final Settings nodeSettings() { return Settings.builder() // allowlist 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(); } diff --git a/modules/reindex/src/yamlRestTest/resources/rest-api-spec/test/reindex/20_validation.yml b/modules/reindex/src/yamlRestTest/resources/rest-api-spec/test/reindex/20_validation.yml index 876d100e0bc3c..15e2397099b65 100644 --- a/modules/reindex/src/yamlRestTest/resources/rest-api-spec/test/reindex/20_validation.yml +++ b/modules/reindex/src/yamlRestTest/resources/rest-api-spec/test/reindex/20_validation.yml @@ -306,9 +306,9 @@ index: dest --- -"unwhitelisted remote host fails": +"unallowlisted remote host fails": - do: - catch: /\[badremote:9200\] not allowlisted in reindex.remote.whitelist/ + catch: /\[badremote:9200\] not allowlisted in reindex.remote.allowlist/ reindex: body: source: