From ebdffbb2bf057096028aa3e6321be8a2ebc08ec4 Mon Sep 17 00:00:00 2001 From: Ankit Kala Date: Tue, 22 Aug 2023 18:16:26 +0530 Subject: [PATCH] [BUG] Add support to clear archived index settings (#9019) * Add support to clear archived index setting Signed-off-by: Ankit Kala --- CHANGELOG.md | 1 + .../settings/ArchivedIndexSettingsIT.java | 129 ++++++++++++++++++ .../MetadataUpdateSettingsService.java | 10 +- .../common/settings/IndexScopedSettings.java | 2 + .../opensearch/common/settings/Settings.java | 5 +- .../org/opensearch/index/IndexSettings.java | 4 +- .../common/settings/SettingsTests.java | 14 ++ 7 files changed, 161 insertions(+), 4 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 06b47e7453b3c..cf70236502702 100644 --- 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/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/settings/ArchivedIndexSettingsIT.java new file mode 100644 index 0000000000000..20b0a6175c562 --- /dev/null +++ b/server/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> nodePlugins() { + return installPlugin ? Arrays.asList(DummySettingPlugin.class) : Collections.emptyList(); + } + + public static class DummySettingPlugin extends Plugin { + public static final Setting DUMMY_SETTING = Setting.simpleString( + "index.dummy", + Setting.Property.IndexScope, + Setting.Property.Dynamic + ); + public static final Setting DUMMY_SETTING2 = Setting.simpleString( + "index.dummy2", + Setting.Property.IndexScope, + Setting.Property.Dynamic + ); + + @Override + public List> 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 index d1e9642596cea..524980565fe39 100644 --- 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,12 +136,16 @@ 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: " @@ -148,7 +153,8 @@ public void updateSettings( + " 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 index f14db4354f196..68d02151b50f5 100644 --- 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 INDEX_SETTINGS_KEY_PREDICATE = (s) -> s.startsWith(IndexMetadata.INDEX_SETTING_PREFIX); + public static final Predicate ARCHIVED_SETTINGS_KEY_PREDICATE = (s) -> s.startsWith(ARCHIVED_SETTINGS_PREFIX); + public static final Set> 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 index 8f3bf1fd66b81..ae10f38943e73 100644 --- 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 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 index 9ceb03974166f..ec719c99e163f 100644 --- 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 index 99dc171f5b1de..af4efabb341ee 100644 --- 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");