[Rollup] Disallow index patterns that match the rollup index (#30491)

We should not allow the user to configure index patterns that also match the index which stores the 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. Note: this does not prevent the user from matching against existing rollup indices. That should be prevented by the field-level validation during job creation.
elastic · Jun 5, 2018 · 3aeff08 · 3aeff08
1 parent d7d4d16
commit 3aeff08
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 6 deletions.
diff --git a/...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/RollupJobConfig.java b/...ck/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/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()) {

diff --git a/...llup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java b/...llup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java
@@ -95,8 +95,8 @@ protected void masterOperation(PutRollupJobAction.Request request, ClusterState
         XPackPlugin.checkReadyForXPackCustomMetadata(clusterState);
 
         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<FieldCapabilitiesResponse>() {
             @Override

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
@@ -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
@@ -188,4 +188,3 @@ setup:
             ]
           }
 
-