From 432f5788076270fab3ea7d946b03b3a0ded694e5 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Wed, 18 Feb 2015 10:21:17 -0500 Subject: [PATCH] Internal: refactor settings filtering Refactor how settings filters are handled. Instead of specifying settings filters as filtering class, settings filters are now specified as list of settings that needs to be filtered out. Regex syntax is supported. This is breaking change and will require small change in plugins that are using settingsFilters. This change is needed in order to simplify cluster state diff implementation. Contributes to #6295 --- .../cluster/node/info/NodesInfoResponse.java | 9 +- .../get/TransportGetSettingsAction.java | 2 +- .../elasticsearch/cluster/ClusterState.java | 16 +-- .../common/settings/ImmutableSettings.java | 5 +- .../common/settings/SettingsFilter.java | 82 +++++++++-- .../node/info/RestNodesInfoAction.java | 3 +- .../reroute/RestClusterRerouteAction.java | 3 +- .../cluster/state/RestClusterStateAction.java | 3 +- .../settings/SettingsFilteringTests.java | 115 ++++++++++++++++ .../common/settings/SettingsFilterTests.java | 128 ++++++++++++++++++ .../elasticsearch/rest/FakeRestRequest.java | 21 ++- 11 files changed, 337 insertions(+), 50 deletions(-) create mode 100644 src/test/java/org/elasticsearch/cluster/settings/SettingsFilteringTests.java create mode 100644 src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java diff --git a/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java b/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java index 0eb8ac2176f..2b16962b461 100644 --- a/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java +++ b/src/main/java/org/elasticsearch/action/admin/cluster/node/info/NodesInfoResponse.java @@ -37,8 +37,6 @@ import java.util.Map; */ public class NodesInfoResponse extends NodesOperationResponse implements ToXContent { - private SettingsFilter settingsFilter; - public NodesInfoResponse() { } @@ -64,11 +62,6 @@ public class NodesInfoResponse extends NodesOperationResponse implemen } } - public NodesInfoResponse settingsFilter(SettingsFilter settingsFilter) { - this.settingsFilter = settingsFilter; - return this; - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.field("cluster_name", getClusterName().value(), XContentBuilder.FieldCaseConversion.NONE); @@ -102,7 +95,7 @@ public class NodesInfoResponse extends NodesOperationResponse implemen if (nodeInfo.getSettings() != null) { builder.startObject("settings"); - Settings settings = settingsFilter != null ? settingsFilter.filterSettings(nodeInfo.getSettings()) : nodeInfo.getSettings(); + Settings settings = nodeInfo.getSettings(); settings.toXContent(builder, params); builder.endObject(); } diff --git a/src/main/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsAction.java b/src/main/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsAction.java index 1ad60c477a9..fd01607e954 100644 --- a/src/main/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsAction.java +++ b/src/main/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsAction.java @@ -86,7 +86,7 @@ public class TransportGetSettingsAction extends TransportMasterNodeReadOperation continue; } - Settings settings = settingsFilter.filterSettings(indexMetaData.settings()); + Settings settings = SettingsFilter.filterSettings(settingsFilter.getPatterns(), indexMetaData.settings()); if (!CollectionUtils.isEmpty(request.names())) { ImmutableSettings.Builder settingsBuilder = ImmutableSettings.builder(); for (Map.Entry entry : settings.getAsMap().entrySet()) { diff --git a/src/main/java/org/elasticsearch/cluster/ClusterState.java b/src/main/java/org/elasticsearch/cluster/ClusterState.java index fae22af1c26..8d50af41302 100644 --- a/src/main/java/org/elasticsearch/cluster/ClusterState.java +++ b/src/main/java/org/elasticsearch/cluster/ClusterState.java @@ -33,7 +33,7 @@ import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.routing.*; -import org.elasticsearch.cluster.routing.allocation.AllocationExplanation; + import org.elasticsearch.cluster.routing.allocation.RoutingAllocation; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; @@ -44,7 +44,6 @@ import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsFilter; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; @@ -134,8 +133,6 @@ public class ClusterState implements ToXContent { // built on demand private volatile RoutingNodes routingNodes; - private SettingsFilter settingsFilter; - private volatile ClusterStateStatus status; public ClusterState(long version, ClusterState state) { @@ -234,11 +231,6 @@ public class ClusterState implements ToXContent { return routingNodes; } - public ClusterState settingsFilter(SettingsFilter settingsFilter) { - this.settingsFilter = settingsFilter; - return this; - } - public String prettyPrint() { StringBuilder sb = new StringBuilder(); sb.append("version: ").append(version).append("\n"); @@ -385,9 +377,6 @@ public class ClusterState implements ToXContent { builder.startObject("settings"); Settings settings = templateMetaData.settings(); - if (settingsFilter != null) { - settings = settingsFilter.filterSettings(settings); - } settings.toXContent(builder, params); builder.endObject(); @@ -418,9 +407,6 @@ public class ClusterState implements ToXContent { builder.startObject("settings"); Settings settings = indexMetaData.settings(); - if (settingsFilter != null) { - settings = settingsFilter.filterSettings(settings); - } settings.toXContent(builder, params); builder.endObject(); diff --git a/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java b/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java index 39ddfeac9ed..7758acb0432 100644 --- a/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java +++ b/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java @@ -648,12 +648,13 @@ public class ImmutableSettings implements Settings { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + Settings settings = SettingsFilter.filterSettings(params, this); if (!params.paramAsBoolean("flat_settings", false)) { - for (Map.Entry entry : getAsStructuredMap().entrySet()) { + for (Map.Entry entry : settings.getAsStructuredMap().entrySet()) { builder.field(entry.getKey(), entry.getValue()); } } else { - for (Map.Entry entry : getAsMap().entrySet()) { + for (Map.Entry entry : settings.getAsMap().entrySet()) { builder.field(entry.getKey(), entry.getValue(), XContentBuilder.FieldCaseConversion.NONE); } } diff --git a/src/main/java/org/elasticsearch/common/settings/SettingsFilter.java b/src/main/java/org/elasticsearch/common/settings/SettingsFilter.java index d7332f0072f..f02ebd3f54a 100644 --- a/src/main/java/org/elasticsearch/common/settings/SettingsFilter.java +++ b/src/main/java/org/elasticsearch/common/settings/SettingsFilter.java @@ -18,41 +18,95 @@ */ package org.elasticsearch.common.settings; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.regex.Regex; +import org.elasticsearch.common.xcontent.ToXContent.Params; +import org.elasticsearch.rest.RestRequest; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; import java.util.concurrent.CopyOnWriteArrayList; +import static com.google.common.collect.Lists.newArrayList; + /** * */ public class SettingsFilter extends AbstractComponent { + /** + * Can be used to specify settings filter that will be used to filter out matching settings in toXContent method + */ + public static String SETTINGS_FILTER_PARAM = "settings_filter"; - public static interface Filter { - - void filter(ImmutableSettings.Builder settings); - } - - private final CopyOnWriteArrayList filters = new CopyOnWriteArrayList<>(); + private final CopyOnWriteArrayList patterns = new CopyOnWriteArrayList<>(); @Inject public SettingsFilter(Settings settings) { super(settings); } - public void addFilter(Filter filter) { - filters.add(filter); + /** + * Adds a new simple pattern to the list of filters + * + * @param pattern + */ + public void addFilter(String pattern) { + patterns.add(pattern); } - public void removeFilter(Filter filter) { - filters.remove(filter); + /** + * Removes a simple pattern from the list of filters + * + * @param pattern + */ + public void removeFilter(String pattern) { + patterns.remove(pattern); } - public Settings filterSettings(Settings settings) { + public String getPatterns() { + return Strings.collectionToDelimitedString(patterns, ","); + } + + public void addFilterSettingParams(RestRequest request) { + if (patterns.isEmpty() == false) { + request.params().put(SETTINGS_FILTER_PARAM, getPatterns()); + } + } + + public static Settings filterSettings(Params params, Settings settings) { + String patterns = params.param(SETTINGS_FILTER_PARAM); + Settings filteredSettings = settings; + if (patterns != null && patterns.isEmpty() == false) { + filteredSettings = SettingsFilter.filterSettings(patterns, filteredSettings); + } + return filteredSettings; + } + + public static Settings filterSettings(String patterns, Settings settings) { + String[] patternArray = Strings.delimitedListToStringArray(patterns, ","); ImmutableSettings.Builder builder = ImmutableSettings.settingsBuilder().put(settings); - for (Filter filter : filters) { - filter.filter(builder); + List simpleMatchPatternList = newArrayList(); + for (String pattern : patternArray) { + if (Regex.isSimpleMatchPattern(pattern)) { + simpleMatchPatternList.add(pattern); + } else { + builder.remove(pattern); + } + } + if (!simpleMatchPatternList.isEmpty()) { + String[] simpleMatchPatterns = simpleMatchPatternList.toArray(new String[simpleMatchPatternList.size()]); + Iterator> iterator = builder.internalMap().entrySet().iterator(); + while (iterator.hasNext()) { + Map.Entry current = iterator.next(); + if (Regex.simpleMatch(simpleMatchPatterns, current.getKey())) { + iterator.remove(); + } + } } return builder.build(); } -} +} \ No newline at end of file diff --git a/src/main/java/org/elasticsearch/rest/action/admin/cluster/node/info/RestNodesInfoAction.java b/src/main/java/org/elasticsearch/rest/action/admin/cluster/node/info/RestNodesInfoAction.java index e058f2cf216..dbda82ff387 100644 --- a/src/main/java/org/elasticsearch/rest/action/admin/cluster/node/info/RestNodesInfoAction.java +++ b/src/main/java/org/elasticsearch/rest/action/admin/cluster/node/info/RestNodesInfoAction.java @@ -99,11 +99,12 @@ public class RestNodesInfoAction extends BaseRestHandler { nodesInfoRequest.plugins(metrics.contains("plugins")); } + settingsFilter.addFilterSettingParams(request); + client.admin().cluster().nodesInfo(nodesInfoRequest, new RestBuilderListener(channel) { @Override public RestResponse buildResponse(NodesInfoResponse response, XContentBuilder builder) throws Exception { - response.settingsFilter(settingsFilter); builder.startObject(); response.toXContent(builder, request); builder.endObject(); diff --git a/src/main/java/org/elasticsearch/rest/action/admin/cluster/reroute/RestClusterRerouteAction.java b/src/main/java/org/elasticsearch/rest/action/admin/cluster/reroute/RestClusterRerouteAction.java index 43f35a7d6b9..489acf93db1 100644 --- a/src/main/java/org/elasticsearch/rest/action/admin/cluster/reroute/RestClusterRerouteAction.java +++ b/src/main/java/org/elasticsearch/rest/action/admin/cluster/reroute/RestClusterRerouteAction.java @@ -71,7 +71,8 @@ public class RestClusterRerouteAction extends BaseRestHandler { if (request.param("metric") == null) { request.params().put("metric", DEFAULT_METRICS); } - response.getState().settingsFilter(settingsFilter).toXContent(builder, request); + settingsFilter.addFilterSettingParams(request); + response.getState().toXContent(builder, request); builder.endObject(); if (clusterRerouteRequest.explain()) { assert response.getExplanations() != null; diff --git a/src/main/java/org/elasticsearch/rest/action/admin/cluster/state/RestClusterStateAction.java b/src/main/java/org/elasticsearch/rest/action/admin/cluster/state/RestClusterStateAction.java index 772eceb6666..4826bb9cff0 100644 --- a/src/main/java/org/elasticsearch/rest/action/admin/cluster/state/RestClusterStateAction.java +++ b/src/main/java/org/elasticsearch/rest/action/admin/cluster/state/RestClusterStateAction.java @@ -76,13 +76,14 @@ public class RestClusterStateAction extends BaseRestHandler { clusterStateRequest.metaData(metrics.contains(ClusterState.Metric.METADATA)); clusterStateRequest.blocks(metrics.contains(ClusterState.Metric.BLOCKS)); } + settingsFilter.addFilterSettingParams(request); client.admin().cluster().state(clusterStateRequest, new RestBuilderListener(channel) { @Override public RestResponse buildResponse(ClusterStateResponse response, XContentBuilder builder) throws Exception { builder.startObject(); builder.field(Fields.CLUSTER_NAME, response.getClusterName().value()); - response.getState().settingsFilter(settingsFilter).toXContent(builder, request); + response.getState().toXContent(builder, request); builder.endObject(); return new BytesRestResponse(RestStatus.OK, builder); } diff --git a/src/test/java/org/elasticsearch/cluster/settings/SettingsFilteringTests.java b/src/test/java/org/elasticsearch/cluster/settings/SettingsFilteringTests.java new file mode 100644 index 00000000000..515122c9f20 --- /dev/null +++ b/src/test/java/org/elasticsearch/cluster/settings/SettingsFilteringTests.java @@ -0,0 +1,115 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.cluster.settings; + +import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; +import org.elasticsearch.common.inject.AbstractModule; +import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.inject.Module; +import org.elasticsearch.common.settings.ImmutableSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsFilter; +import org.elasticsearch.plugins.AbstractPlugin; +import org.elasticsearch.test.ElasticsearchIntegrationTest; +import org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope; +import org.junit.Test; + +import java.util.Collection; + +import static com.google.common.collect.Lists.newArrayList; +import static org.elasticsearch.test.ElasticsearchIntegrationTest.Scope.SUITE; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; + +@ClusterScope(scope = SUITE, numDataNodes = 1) +public class SettingsFilteringTests extends ElasticsearchIntegrationTest { + + @Override + protected Settings nodeSettings(int nodeOrdinal) { + return ImmutableSettings.settingsBuilder() + .put(super.nodeSettings(nodeOrdinal)) + .put("plugin.types", SettingsFilteringPlugin.class.getName()) + .build(); + } + + public static class SettingsFilteringPlugin extends AbstractPlugin { + /** + * The name of the plugin. + */ + @Override + public String name() { + return "settings-filtering"; + } + + /** + * The description of the plugin. + */ + @Override + public String description() { + return "Settings Filtering Plugin"; + } + + @Override + public Collection> indexModules() { + Collection> modules = newArrayList(); + modules.add(SettingsFilteringModule.class); + return modules; + } + } + + public static class SettingsFilteringModule extends AbstractModule { + + @Override + protected void configure() { + bind(SettingsFilteringService.class).asEagerSingleton(); + } + } + + public static class SettingsFilteringService { + @Inject + public SettingsFilteringService(SettingsFilter settingsFilter) { + settingsFilter.addFilter("index.filter_test.foo"); + settingsFilter.addFilter("index.filter_test.bar*"); + } + } + + + @Test + public void testSettingsFiltering() { + + assertAcked(client().admin().indices().prepareCreate("test-idx").setSettings(ImmutableSettings.builder() + .put("filter_test.foo", "test") + .put("filter_test.bar1", "test") + .put("filter_test.bar2", "test") + .put("filter_test.notbar", "test") + .put("filter_test.notfoo", "test") + .build()).get()); + GetSettingsResponse response = client().admin().indices().prepareGetSettings("test-idx").get(); + Settings settings = response.getIndexToSettings().get("test-idx"); + + assertThat(settings.get("index.filter_test.foo"), nullValue()); + assertThat(settings.get("index.filter_test.bar1"), nullValue()); + assertThat(settings.get("index.filter_test.bar2"), nullValue()); + assertThat(settings.get("index.filter_test.notbar"), equalTo("test")); + assertThat(settings.get("index.filter_test.notfoo"), equalTo("test")); + } + +} diff --git a/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java b/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java new file mode 100644 index 00000000000..abaf526b837 --- /dev/null +++ b/src/test/java/org/elasticsearch/common/settings/SettingsFilterTests.java @@ -0,0 +1,128 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.common.settings; + +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.rest.FakeRestRequest; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.test.ElasticsearchTestCase; +import org.junit.Test; + +import java.io.IOException; + +import static org.hamcrest.CoreMatchers.equalTo; + +public class SettingsFilterTests extends ElasticsearchTestCase { + + @Test + public void testAddingAndRemovingFilters() { + SettingsFilter settingsFilter = new SettingsFilter(ImmutableSettings.EMPTY); + settingsFilter.addFilter("foo"); + settingsFilter.addFilter("bar"); + settingsFilter.addFilter("baz"); + assertThat(settingsFilter.getPatterns(), equalTo("foo,bar,baz")); + + settingsFilter.removeFilter("bar"); + assertThat(settingsFilter.getPatterns(), equalTo("foo,baz")); + + settingsFilter.removeFilter("bar"); + settingsFilter.removeFilter("foo"); + settingsFilter.removeFilter("baz"); + + assertThat(settingsFilter.getPatterns(), equalTo("")); + } + + @Test + public void testSettingsFiltering() throws IOException { + + testFiltering(ImmutableSettings.builder() + .put("foo", "foo_test") + .put("foo1", "foo1_test") + .put("bar", "bar_test") + .put("bar1", "bar1_test") + .put("bar.2", "bar2_test") + .build(), + ImmutableSettings.builder() + .put("foo1", "foo1_test") + .build(), + "foo,bar*" + ); + + testFiltering(ImmutableSettings.builder() + .put("foo", "foo_test") + .put("foo1", "foo1_test") + .put("bar", "bar_test") + .put("bar1", "bar1_test") + .put("bar.2", "bar2_test") + .build(), + ImmutableSettings.builder() + .put("foo", "foo_test") + .put("foo1", "foo1_test") + .build(), + "bar*" + ); + + testFiltering(ImmutableSettings.builder() + .put("foo", "foo_test") + .put("foo1", "foo1_test") + .put("bar", "bar_test") + .put("bar1", "bar1_test") + .put("bar.2", "bar2_test") + .build(), + ImmutableSettings.builder() + .build(), + "foo,bar*,foo*" + ); + + testFiltering(ImmutableSettings.builder() + .put("foo", "foo_test") + .put("bar", "bar_test") + .put("baz", "baz_test") + .build(), + ImmutableSettings.builder() + .put("foo", "foo_test") + .put("bar", "bar_test") + .put("baz", "baz_test") + .build() + ); + } + + private void testFiltering(Settings source, Settings filtered, String... patterns) throws IOException { + SettingsFilter settingsFilter = new SettingsFilter(ImmutableSettings.EMPTY); + for (String pattern : patterns) { + settingsFilter.addFilter(pattern); + } + + // Test using direct filtering + Settings filteredSettings = SettingsFilter.filterSettings(settingsFilter.getPatterns(), source); + assertThat(filteredSettings.getAsMap().entrySet(), equalTo(filtered.getAsMap().entrySet())); + + // Test using toXContent filtering + RestRequest request = new FakeRestRequest(); + settingsFilter.addFilterSettingParams(request); + XContentBuilder xContentBuilder = XContentBuilder.builder(JsonXContent.jsonXContent); + xContentBuilder.startObject(); + source.toXContent(xContentBuilder, request); + xContentBuilder.endObject(); + String filteredSettingsString = xContentBuilder.string(); + filteredSettings = ImmutableSettings.builder().loadFromSource(filteredSettingsString).build(); + assertThat(filteredSettings.getAsMap().entrySet(), equalTo(filtered.getAsMap().entrySet())); + } +} diff --git a/src/test/java/org/elasticsearch/rest/FakeRestRequest.java b/src/test/java/org/elasticsearch/rest/FakeRestRequest.java index 60612ac3770..571e54e1504 100644 --- a/src/test/java/org/elasticsearch/rest/FakeRestRequest.java +++ b/src/test/java/org/elasticsearch/rest/FakeRestRequest.java @@ -24,19 +24,22 @@ import org.elasticsearch.common.bytes.BytesReference; import java.util.HashMap; import java.util.Map; -class FakeRestRequest extends RestRequest { +public class FakeRestRequest extends RestRequest { private final Map headers; - FakeRestRequest() { + private final Map params; + + public FakeRestRequest() { this(new HashMap(), new HashMap()); } - FakeRestRequest(Map headers, Map context) { + public FakeRestRequest(Map headers, Map context) { this.headers = headers; for (Map.Entry entry : context.entrySet()) { putInContext(entry.getKey(), entry.getValue()); } + this.params = new HashMap<>(); } @Override @@ -81,21 +84,25 @@ class FakeRestRequest extends RestRequest { @Override public boolean hasParam(String key) { - return false; + return params.containsKey(key); } @Override public String param(String key) { - return null; + return params.get(key); } @Override public String param(String key, String defaultValue) { - return null; + String value = params.get(key); + if (value == null) { + return defaultValue; + } + return value; } @Override public Map params() { - return null; + return params; } } \ No newline at end of file