Deprecate setting 'reindex.remote.whitelist' and introduce the altern…

…ative setting 'reindex.remote.allowlist' (opensearch-project#2221) * Add setting reindex.remote.allowlist, and deprecate setting reindex.remote.whitelist Signed-off-by: Tianli Feng <ftianli@amazon.com> * Add unit test for renaming the setting reindex.remote.allowlist Signed-off-by: Tianli Feng <ftianli@amazon.com> * Remove system.out.println() Signed-off-by: Tianli Feng <ftianli@amazon.com> * Adjust format by spotlessApply task Signed-off-by: Tianli Feng <ftianli@amazon.com> * Replace REMOTE_CLUSTER_WHITELIST with REMOTE_CLUSTER_ALLOWLIST Signed-off-by: Tianli Feng <ftianli@amazon.com> * Add a unit test to test final setting value when both settings have got a value Signed-off-by: Tianli Feng <ftianli@amazon.com> * Rename the unit test class name Signed-off-by: Tianli Feng <ftianli@amazon.com> * Remove the Access modifiers public from the constant REMOTE_CLUSTER_WHITELIST Signed-off-by: Tianli Feng <ftianli@amazon.com> * Initialize ReindexPlugin without using the @before method Signed-off-by: Tianli Feng <ftianli@amazon.com> * Rename 'unwhitelisted' to 'unallowlisted' in a yml file used for REST api testing. Signed-off-by: Tianli Feng <ftianli@amazon.com>
kartg · Mar 8, 2022 · 63c75d1 · 63c75d1
1 parent 65debde
commit 63c75d1
Show file tree

Hide file tree

Showing 10 changed files with 103 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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;
     }

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
@@ -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
@@ -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();

diff --git a/...s/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWhitelistTests.java b/...s/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/...es/reindex/src/test/java/org/opensearch/index/reindex/ReindexFromRemoteWithAuthTests.java b/...es/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
@@ -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<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 });
+    }
+
+}
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
@@ -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
@@ -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: