From c660ebef7a58a50cc71b6124ff27e3a1ed6d6e80 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Tue, 8 May 2018 22:01:53 +0000 Subject: [PATCH 1/3] [Rollup] Disallow index patterns that match rollup indices We should not allow the user to configure index patterns that also match rollup indices (either the index specified in the config, or previously created jobs). Matching against rollup indices will almost certainly lead to bad behavior, because the expected fields in the user index won't exist in the rollup (due to renaming). It is also unlikely a user will want to match against a rollup index. For example, it is quite natural for a user to specify `metricbeat-*` as the index pattern, and then store the rollups in `metricbeat-rolled`. This will start throwing errors as soon as the rollup index is created because the indexer will try to search it. --- .../core/rollup/job/RollupJobConfig.java | 14 ++++- .../action/TransportPutRollupJobAction.java | 34 +++++++++++- .../xpack/rollup/config/ConfigTests.java | 31 +++++++++++ .../rest-api-spec/test/rollup/put_job.yml | 53 +++++++++++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java index a799cbe944715..3818ebcf44758 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java @@ -13,6 +13,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.regex.Regex; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ToXContent; @@ -173,7 +174,7 @@ public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params par builder.endObject(); return builder; } - + @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(id); @@ -336,6 +337,17 @@ public RollupJobConfig build() { if (indexPattern == null || indexPattern.isEmpty()) { throw new IllegalArgumentException("An index pattern is mandatory."); } + if (Regex.isMatchAllPattern(indexPattern)) { + throw new IllegalArgumentException("Index pattern must not match all indices (as it would match it's own rollup index"); + } + if (Regex.isSimpleMatchPattern(indexPattern)) { + if (Regex.simpleMatch(indexPattern, rollupIndex)) { + throw new IllegalArgumentException("Index pattern would match rollup index name which is not allowed."); + } + } + if (indexPattern.equals(rollupIndex)) { + throw new IllegalArgumentException("Rollup index may not be the same as the index pattern."); + } if (rollupIndex == null || rollupIndex.isEmpty()) { throw new IllegalArgumentException("A rollup index name is mandatory."); } diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java index 61b3334f0c166..54dd182785954 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java @@ -21,6 +21,7 @@ import org.elasticsearch.action.fieldcaps.FieldCapabilitiesRequest; import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse; import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.master.TransportMasterNodeAction; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterState; @@ -44,10 +45,12 @@ import org.elasticsearch.xpack.core.XPackField; import org.elasticsearch.xpack.core.rollup.RollupField; import org.elasticsearch.xpack.core.rollup.action.PutRollupJobAction; +import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps; import org.elasticsearch.xpack.core.rollup.job.RollupJob; import org.elasticsearch.xpack.core.rollup.job.RollupJobConfig; import org.elasticsearch.xpack.rollup.Rollup; +import java.util.Arrays; import java.util.Map; import java.util.Objects; import java.util.stream.Collectors; @@ -91,9 +94,16 @@ protected void masterOperation(PutRollupJobAction.Request request, ClusterState return; } + // Check to see if the index pattern matches any existing rollup indices + ActionRequestValidationException validationException = validateIndexPattern(request, clusterState); + if (validationException != null) { + listener.onFailure(validationException); + return; + } + FieldCapabilitiesRequest fieldCapsRequest = new FieldCapabilitiesRequest() - .indices(request.getConfig().getIndexPattern()) - .fields(request.getConfig().getAllFields().toArray(new String[0])); + .indices(request.getConfig().getIndexPattern()) + .fields(request.getConfig().getAllFields().toArray(new String[0])); client.fieldCaps(fieldCapsRequest, new ActionListener() { @Override @@ -115,6 +125,26 @@ public void onFailure(Exception e) { }); } + /** + * Ensure that the index pattern doesn't match any pre-existing rollup indices. We don't need to check + * if the pattern would match it's own rollup index because the request already validated that. + */ + private static ActionRequestValidationException validateIndexPattern(PutRollupJobAction.Request request, ClusterState clusterState) { + IndexNameExpressionResolver resolver = new IndexNameExpressionResolver(Settings.EMPTY); + String[] indices = resolver.concreteIndexNames(clusterState, IndicesOptions.strictExpandOpenAndForbidClosed(), + request.getConfig().getIndexPattern()); + + ActionRequestValidationException validationException = new ActionRequestValidationException(); + + Arrays.stream(indices) + .forEach(index -> TransportGetRollupCapsAction.findRollupIndexCaps(index, clusterState.getMetaData().index(index)) + .ifPresent(c -> validationException + .addValidationError("Index pattern [" + request.getConfig().getIndexPattern() + "] matches existing rollup index [" + + index + "] from jobs " + c.getJobCaps().stream().map(RollupJobCaps::getJobID).collect(Collectors.toList())))); + + return validationException.validationErrors().size() > 0 ? validationException : null; + } + private static RollupJob createRollupJob(RollupJobConfig config, ThreadPool threadPool) { // ensure we only filter for the allowed headers Map filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream() diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/config/ConfigTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/config/ConfigTests.java index f22a6c87a3ff1..e465c7883cfdd 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/config/ConfigTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/config/ConfigTests.java @@ -122,6 +122,37 @@ public void testEmptyIndexPattern() { assertThat(e.getMessage(), equalTo("An index pattern is mandatory.")); } + public void testMatchAllIndexPattern() { + RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo"); + job.setIndexPattern("*"); + Exception e = expectThrows(IllegalArgumentException.class, job::build); + assertThat(e.getMessage(), equalTo("Index pattern must not match all indices (as it would match it's own rollup index")); + } + + public void testMatchOwnRollupPatternPrefix() { + RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo"); + job.setIndexPattern("foo-*"); + job.setRollupIndex("foo-rollup"); + Exception e = expectThrows(IllegalArgumentException.class, job::build); + assertThat(e.getMessage(), equalTo("Index pattern would match rollup index name which is not allowed.")); + } + + public void testMatchOwnRollupPatternSuffix() { + RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo"); + job.setIndexPattern("*-rollup"); + job.setRollupIndex("foo-rollup"); + Exception e = expectThrows(IllegalArgumentException.class, job::build); + assertThat(e.getMessage(), equalTo("Index pattern would match rollup index name which is not allowed.")); + } + + public void testIndexPatternIdenticalToRollup() { + RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo"); + job.setIndexPattern("foo"); + job.setRollupIndex("foo"); + Exception e = expectThrows(IllegalArgumentException.class, job::build); + assertThat(e.getMessage(), equalTo("Rollup index may not be the same as the index pattern.")); + } + public void testEmptyRollupIndex() { RollupJobConfig.Builder job = ConfigTestHelpers.getRollupJob("foo"); job.setRollupIndex(""); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml index 080fed7a80ec9..da64dea1dd70a 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml @@ -159,5 +159,58 @@ setup: ] } +--- +"Index pattern matches existing rollup index": + + - do: + headers: + Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser + xpack.rollup.put_job: + id: foo + body: > + { + "index_pattern": "foo", + "rollup_index": "foo_rollup", + "cron": "*/30 * * * * ?", + "page_size" :10, + "groups" : { + "date_histogram": { + "field": "the_field", + "interval": "1h" + } + }, + "metrics": [ + { + "field": "value_field", + "metrics": ["min", "max", "sum"] + } + ] + } + - is_true: acknowledged + - do: + catch: /Index pattern \[foo\*\] matches existing rollup index \[foo_rollup\] from jobs \[foo\]/ + headers: + Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser + xpack.rollup.put_job: + id: foo2 + body: > + { + "index_pattern": "foo*", + "rollup_index": "some_index", + "cron": "*/30 * * * * ?", + "page_size" :10, + "groups" : { + "date_histogram": { + "field": "the_field", + "interval": "1h" + } + }, + "metrics": [ + { + "field": "value_field", + "metrics": ["min", "max", "sum"] + } + ] + } From 733e12db7dd0f31d3201cb481a52efd880b9f0f8 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Fri, 1 Jun 2018 15:38:00 +0000 Subject: [PATCH 2/3] Relax validation to only check for self-matching --- .../action/TransportPutRollupJobAction.java | 30 ----------- .../rest-api-spec/test/rollup/put_job.yml | 54 ------------------- 2 files changed, 84 deletions(-) diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java index 54dd182785954..b8446b55ddb3c 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java @@ -21,7 +21,6 @@ import org.elasticsearch.action.fieldcaps.FieldCapabilitiesRequest; import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse; import org.elasticsearch.action.support.ActionFilters; -import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.master.TransportMasterNodeAction; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterState; @@ -45,12 +44,10 @@ import org.elasticsearch.xpack.core.XPackField; import org.elasticsearch.xpack.core.rollup.RollupField; import org.elasticsearch.xpack.core.rollup.action.PutRollupJobAction; -import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps; import org.elasticsearch.xpack.core.rollup.job.RollupJob; import org.elasticsearch.xpack.core.rollup.job.RollupJobConfig; import org.elasticsearch.xpack.rollup.Rollup; -import java.util.Arrays; import java.util.Map; import java.util.Objects; import java.util.stream.Collectors; @@ -94,13 +91,6 @@ protected void masterOperation(PutRollupJobAction.Request request, ClusterState return; } - // Check to see if the index pattern matches any existing rollup indices - ActionRequestValidationException validationException = validateIndexPattern(request, clusterState); - if (validationException != null) { - listener.onFailure(validationException); - return; - } - FieldCapabilitiesRequest fieldCapsRequest = new FieldCapabilitiesRequest() .indices(request.getConfig().getIndexPattern()) .fields(request.getConfig().getAllFields().toArray(new String[0])); @@ -125,26 +115,6 @@ public void onFailure(Exception e) { }); } - /** - * Ensure that the index pattern doesn't match any pre-existing rollup indices. We don't need to check - * if the pattern would match it's own rollup index because the request already validated that. - */ - private static ActionRequestValidationException validateIndexPattern(PutRollupJobAction.Request request, ClusterState clusterState) { - IndexNameExpressionResolver resolver = new IndexNameExpressionResolver(Settings.EMPTY); - String[] indices = resolver.concreteIndexNames(clusterState, IndicesOptions.strictExpandOpenAndForbidClosed(), - request.getConfig().getIndexPattern()); - - ActionRequestValidationException validationException = new ActionRequestValidationException(); - - Arrays.stream(indices) - .forEach(index -> TransportGetRollupCapsAction.findRollupIndexCaps(index, clusterState.getMetaData().index(index)) - .ifPresent(c -> validationException - .addValidationError("Index pattern [" + request.getConfig().getIndexPattern() + "] matches existing rollup index [" - + index + "] from jobs " + c.getJobCaps().stream().map(RollupJobCaps::getJobID).collect(Collectors.toList())))); - - return validationException.validationErrors().size() > 0 ? validationException : null; - } - private static RollupJob createRollupJob(RollupJobConfig config, ThreadPool threadPool) { // ensure we only filter for the allowed headers Map filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream() diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml index da64dea1dd70a..24da292250290 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/put_job.yml @@ -159,58 +159,4 @@ setup: ] } ---- -"Index pattern matches existing rollup index": - - - do: - headers: - Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser - xpack.rollup.put_job: - id: foo - body: > - { - "index_pattern": "foo", - "rollup_index": "foo_rollup", - "cron": "*/30 * * * * ?", - "page_size" :10, - "groups" : { - "date_histogram": { - "field": "the_field", - "interval": "1h" - } - }, - "metrics": [ - { - "field": "value_field", - "metrics": ["min", "max", "sum"] - } - ] - } - - is_true: acknowledged - - - do: - catch: /Index pattern \[foo\*\] matches existing rollup index \[foo_rollup\] from jobs \[foo\]/ - headers: - Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser - xpack.rollup.put_job: - id: foo2 - body: > - { - "index_pattern": "foo*", - "rollup_index": "some_index", - "cron": "*/30 * * * * ?", - "page_size" :10, - "groups" : { - "date_histogram": { - "field": "the_field", - "interval": "1h" - } - }, - "metrics": [ - { - "field": "value_field", - "metrics": ["min", "max", "sum"] - } - ] - } From 2c3185cf8ab76c24f5a5c6b2a45673ba30837dcc Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Mon, 4 Jun 2018 14:13:17 +0000 Subject: [PATCH 3/3] [TEST] ensure random jobs never have same index pattern and rollup index --- .../elasticsearch/xpack/core/rollup/ConfigTestHelpers.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java index 7522f474e77b2..3d82ac118f503 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java @@ -27,8 +27,9 @@ public static RollupJobConfig.Builder getRollupJob(String jobId) { builder.setId(jobId); builder.setCron(getCronString()); builder.setTimeout(new TimeValue(ESTestCase.randomIntBetween(1,100))); - builder.setIndexPattern(ESTestCase.randomAlphaOfLengthBetween(1,10)); - builder.setRollupIndex(ESTestCase.randomAlphaOfLengthBetween(1,10)); + String indexPattern = ESTestCase.randomAlphaOfLengthBetween(1,10); + builder.setIndexPattern(indexPattern); + builder.setRollupIndex("rollup_" + indexPattern); // to ensure the index pattern != rollup index builder.setGroupConfig(ConfigTestHelpers.getGroupConfig().build()); builder.setPageSize(ESTestCase.randomIntBetween(1,10)); if (ESTestCase.randomBoolean()) {