[BUG] Add support to clear archived index settings (opensearch-projec…

…t#9019) * Add support to clear archived index setting Signed-off-by: Ankit Kala <[email protected]> Signed-off-by: Kiran Reddy <[email protected]>
kkmr · Aug 28, 2023 · bd590d8 · bd590d8
1 parent b336b7e
commit bd590d8
Show file tree

Hide file tree

Showing 7 changed files with 161 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -72,6 +72,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Fixed
 - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827))
 - Fix compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944))
+- Add support to clear archived index setting ([#9019](https://github.com/opensearch-project/OpenSearch/pull/9019))
 
 ### Security
 

diff --git a/...src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java b/...src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java
@@ -0,0 +1,129 @@
+/*
+ * 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.indices.settings;
+
+import org.opensearch.common.settings.Setting;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.plugins.Plugin;
+import org.opensearch.test.InternalTestCluster;
+import org.opensearch.test.OpenSearchIntegTestCase;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+
+import static org.hamcrest.Matchers.startsWith;
+
+@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, supportsDedicatedMasters = false)
+public class ArchivedIndexSettingsIT extends OpenSearchIntegTestCase {
+    private volatile boolean installPlugin;
+
+    public void testArchiveSettings() throws Exception {
+        installPlugin = true;
+        // Set up the cluster with an index containing dummy setting(owned by dummy plugin)
+        String oldClusterManagerNode = internalCluster().startClusterManagerOnlyNode();
+        String oldDataNode = internalCluster().startDataOnlyNode();
+        assertEquals(2, internalCluster().numDataAndClusterManagerNodes());
+        createIndex("test");
+        ensureYellow();
+        // Add a dummy setting
+        client().admin()
+            .indices()
+            .prepareUpdateSettings("test")
+            .setSettings(Settings.builder().put("index.dummy", "foobar").put("index.dummy2", "foobar"))
+            .execute()
+            .actionGet();
+
+        // Remove dummy plugin and replace the cluster manager node so that the stale plugin setting moves to "archived".
+        installPlugin = false;
+        String newClusterManagerNode = internalCluster().startClusterManagerOnlyNode();
+        internalCluster().stopRandomNode(InternalTestCluster.nameFilter(oldClusterManagerNode));
+        internalCluster().restartNode(newClusterManagerNode);
+
+        // Verify that archived settings exists.
+        assertTrue(
+            client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy")
+        );
+        assertTrue(
+            client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy2")
+        );
+
+        // Archived setting update should fail on open index.
+        IllegalArgumentException exception = expectThrows(
+            IllegalArgumentException.class,
+            () -> client().admin()
+                .indices()
+                .prepareUpdateSettings("test")
+                .setSettings(Settings.builder().putNull("archived.index.dummy"))
+                .execute()
+                .actionGet()
+        );
+        assertThat(
+            exception.getMessage(),
+            startsWith("Can't update non dynamic settings [[archived.index.dummy]] for open indices [[test")
+        );
+
+        // close the index.
+        client().admin().indices().prepareClose("test").get();
+
+        // Remove archived.index.dummy explicitly.
+        assertTrue(
+            client().admin()
+                .indices()
+                .prepareUpdateSettings("test")
+                .setSettings(Settings.builder().putNull("archived.index.dummy"))
+                .execute()
+                .actionGet()
+                .isAcknowledged()
+        );
+
+        // Remove archived.index.dummy2 using wildcard.
+        assertTrue(
+            client().admin()
+                .indices()
+                .prepareUpdateSettings("test")
+                .setSettings(Settings.builder().putNull("archived.*"))
+                .execute()
+                .actionGet()
+                .isAcknowledged()
+        );
+
+        // Verify that archived settings are cleaned up successfully.
+        assertFalse(
+            client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy")
+        );
+        assertFalse(
+            client().admin().indices().prepareGetSettings("test").get().getIndexToSettings().get("test").hasValue("archived.index.dummy2")
+        );
+    }
+
+    @Override
+    protected Collection<Class<? extends Plugin>> nodePlugins() {
+        return installPlugin ? Arrays.asList(DummySettingPlugin.class) : Collections.emptyList();
+    }
+
+    public static class DummySettingPlugin extends Plugin {
+        public static final Setting<String> DUMMY_SETTING = Setting.simpleString(
+            "index.dummy",
+            Setting.Property.IndexScope,
+            Setting.Property.Dynamic
+        );
+        public static final Setting<String> DUMMY_SETTING2 = Setting.simpleString(
+            "index.dummy2",
+            Setting.Property.IndexScope,
+            Setting.Property.Dynamic
+        );
+
+        @Override
+        public List<Setting<?>> getSettings() {
+            return Arrays.asList(DUMMY_SETTING, DUMMY_SETTING2);
+        }
+    }
+}
diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java
@@ -73,6 +73,7 @@
 import java.util.Set;
 
 import static org.opensearch.action.support.ContextPreservingActionListener.wrapPreservingContext;
+import static org.opensearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX;
 import static org.opensearch.index.IndexSettings.same;
 
 /**
@@ -135,20 +136,25 @@ public void updateSettings(
         indexScopedSettings.validate(
             normalizedSettings.filter(s -> Regex.isSimpleMatchPattern(s) == false), // don't validate wildcards
             false, // don't validate dependencies here we check it below never allow to change the number of shards
+            false,
+            true, // Ignore archived setting.
             true
         ); // validate internal or private index settings
         for (String key : normalizedSettings.keySet()) {
             Setting setting = indexScopedSettings.get(key);
             boolean isWildcard = setting == null && Regex.isSimpleMatchPattern(key);
+            boolean isArchived = key.startsWith(ARCHIVED_SETTINGS_PREFIX);
             assert setting != null // we already validated the normalized settings
+                || isArchived
                 || (isWildcard && normalizedSettings.hasValue(key) == false) : "unknown setting: "
                     + key
                     + " isWildcard: "
                     + isWildcard
                     + " hasValue: "
                     + normalizedSettings.hasValue(key);
             settingsForClosedIndices.copy(key, normalizedSettings);
-            if (isWildcard || setting.isDynamic()) {
+            // Only allow dynamic settings and wildcards for open indices. Skip archived settings.
+            if (isArchived == false && (isWildcard || setting.isDynamic())) {
                 settingsForOpenIndices.copy(key, normalizedSettings);
             } else {
                 skippedSettings.add(key);
@@ -308,6 +314,8 @@ public ClusterState execute(ClusterState currentState) {
                                 Settings finalSettings = indexSettings.build();
                                 indexScopedSettings.validate(
                                     finalSettings.filter(k -> indexScopedSettings.isPrivateSetting(k) == false),
+                                    true,
+                                    false,
                                     true
                                 );
                                 metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(finalSettings));

diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java
@@ -76,6 +76,8 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
 
     public static final Predicate<String> INDEX_SETTINGS_KEY_PREDICATE = (s) -> s.startsWith(IndexMetadata.INDEX_SETTING_PREFIX);
 
+    public static final Predicate<String> ARCHIVED_SETTINGS_KEY_PREDICATE = (s) -> s.startsWith(ARCHIVED_SETTINGS_PREFIX);
+
     public static final Set<Setting<?>> BUILT_IN_INDEX_SETTINGS = Collections.unmodifiableSet(
         new HashSet<>(
             Arrays.asList(

diff --git a/server/src/main/java/org/opensearch/common/settings/Settings.java b/server/src/main/java/org/opensearch/common/settings/Settings.java
@@ -88,6 +88,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import static org.opensearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX;
 import static org.opensearch.common.unit.TimeValue.parseTimeValue;
 import static org.opensearch.core.common.unit.ByteSizeValue.parseBytesSizeValue;
 
@@ -1217,7 +1218,7 @@ public boolean shouldRemoveMissingPlaceholder(String placeholderName) {
         }
 
         /**
-         * Checks that all settings in the builder start with the specified prefix.
+         * Checks that all settings(except archived settings and wildcards) in the builder start with the specified prefix.
          *
          * If a setting doesn't start with the prefix, the builder appends the prefix to such setting.
          */
@@ -1227,7 +1228,7 @@ public Builder normalizePrefix(String prefix) {
             while (iterator.hasNext()) {
                 Map.Entry<String, Object> entry = iterator.next();
                 String key = entry.getKey();
-                if (key.startsWith(prefix) == false && key.endsWith("*") == false) {
+                if (key.startsWith(prefix) == false && key.endsWith("*") == false && key.startsWith(ARCHIVED_SETTINGS_PREFIX) == false) {
                     replacements.put(prefix + key, entry.getValue());
                     iterator.remove();
                 }

diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java
@@ -1147,7 +1147,9 @@ public synchronized boolean updateIndexMetadata(IndexMetadata indexMetadata) {
      */
     public static boolean same(final Settings left, final Settings right) {
         return left.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE)
-            .equals(right.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE));
+            .equals(right.filter(IndexScopedSettings.INDEX_SETTINGS_KEY_PREDICATE))
+            && left.filter(IndexScopedSettings.ARCHIVED_SETTINGS_KEY_PREDICATE)
+                .equals(right.filter(IndexScopedSettings.ARCHIVED_SETTINGS_KEY_PREDICATE));
     }
 
     /**

diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsTests.java
@@ -301,6 +301,20 @@ public void testPrefixNormalization() {
         assertThat(settings.get("foo.test"), equalTo("test"));
     }
 
+    public void testPrefixNormalizationArchived() {
+        Settings settings = Settings.builder().put("archived.foo.bar", "baz").normalizePrefix("foo.").build();
+
+        assertThat(settings.size(), equalTo(1));
+        assertThat(settings.get("foo.archived.foo.bar"), nullValue());
+        assertThat(settings.get("archived.foo.bar"), equalTo("baz"));
+
+        settings = Settings.builder().put("archived.foo.*", "baz").normalizePrefix("foo.").build();
+
+        assertThat(settings.size(), equalTo(1));
+        assertThat(settings.get("foo.archived.foo.*"), nullValue());
+        assertThat(settings.get("archived.foo.*"), equalTo("baz"));
+    }
+
     public void testFilteredMap() {
         Settings.Builder builder = Settings.builder();
         builder.put("a", "a1");