Remove timezone validation on rollup range queries (#40647)

We enforced the timezone of range queries when using the rollup
search endpoint, but this validation is not needed.  Since
rollup dates are stored in UTC, and range queries are always
converted to UTC (even if specifying a `time_zone`) the validation
is not needed and can prevent legitimate queries from running.
This commit is contained in:
Zachary Tong 2019-04-02 14:24:41 -04:00 committed by Zachary Tong
parent 4842d7fb7d
commit abbfc75052
2 changed files with 10 additions and 37 deletions

View File

@ -56,17 +56,14 @@ import org.elasticsearch.transport.TransportService;
import org.elasticsearch.xpack.core.rollup.RollupField; import org.elasticsearch.xpack.core.rollup.RollupField;
import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps; import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps;
import org.elasticsearch.xpack.core.rollup.action.RollupSearchAction; import org.elasticsearch.xpack.core.rollup.action.RollupSearchAction;
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
import org.elasticsearch.xpack.rollup.Rollup; import org.elasticsearch.xpack.rollup.Rollup;
import org.elasticsearch.xpack.rollup.RollupJobIdentifierUtils; import org.elasticsearch.xpack.rollup.RollupJobIdentifierUtils;
import org.elasticsearch.xpack.rollup.RollupRequestTranslator; import org.elasticsearch.xpack.rollup.RollupRequestTranslator;
import org.elasticsearch.xpack.rollup.RollupResponseTranslator; import org.elasticsearch.xpack.rollup.RollupResponseTranslator;
import org.joda.time.DateTimeZone;
import java.io.IOException; import java.io.IOException;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Arrays; import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet; import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Objects; import java.util.Objects;
@ -286,11 +283,8 @@ public class TransportRollupSearchAction extends TransportAction<SearchRequest,
} else if (builder.getWriteableName().equals(RangeQueryBuilder.NAME)) { } else if (builder.getWriteableName().equals(RangeQueryBuilder.NAME)) {
RangeQueryBuilder range = (RangeQueryBuilder) builder; RangeQueryBuilder range = (RangeQueryBuilder) builder;
String fieldName = range.fieldName(); String fieldName = range.fieldName();
// Many range queries don't include the timezone because the default is UTC, but the query
// builder will return null so we need to set it here
String timeZone = range.timeZone() == null ? DateTimeZone.UTC.toString() : range.timeZone();
String rewrittenFieldName = rewriteFieldName(jobCaps, RangeQueryBuilder.NAME, fieldName, timeZone); String rewrittenFieldName = rewriteFieldName(jobCaps, RangeQueryBuilder.NAME, fieldName);
RangeQueryBuilder rewritten = new RangeQueryBuilder(rewrittenFieldName) RangeQueryBuilder rewritten = new RangeQueryBuilder(rewrittenFieldName)
.from(range.from()) .from(range.from())
.to(range.to()) .to(range.to())
@ -306,12 +300,12 @@ public class TransportRollupSearchAction extends TransportAction<SearchRequest,
} else if (builder.getWriteableName().equals(TermQueryBuilder.NAME)) { } else if (builder.getWriteableName().equals(TermQueryBuilder.NAME)) {
TermQueryBuilder term = (TermQueryBuilder) builder; TermQueryBuilder term = (TermQueryBuilder) builder;
String fieldName = term.fieldName(); String fieldName = term.fieldName();
String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName, null); String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName);
return new TermQueryBuilder(rewrittenFieldName, term.value()); return new TermQueryBuilder(rewrittenFieldName, term.value());
} else if (builder.getWriteableName().equals(TermsQueryBuilder.NAME)) { } else if (builder.getWriteableName().equals(TermsQueryBuilder.NAME)) {
TermsQueryBuilder terms = (TermsQueryBuilder) builder; TermsQueryBuilder terms = (TermsQueryBuilder) builder;
String fieldName = terms.fieldName(); String fieldName = terms.fieldName();
String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName, null); String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName);
return new TermsQueryBuilder(rewrittenFieldName, terms.values()); return new TermsQueryBuilder(rewrittenFieldName, terms.values());
} else if (builder.getWriteableName().equals(MatchAllQueryBuilder.NAME)) { } else if (builder.getWriteableName().equals(MatchAllQueryBuilder.NAME)) {
// no-op // no-op
@ -321,11 +315,7 @@ public class TransportRollupSearchAction extends TransportAction<SearchRequest,
} }
} }
private static String rewriteFieldName(Set<RollupJobCaps> jobCaps, private static String rewriteFieldName(Set<RollupJobCaps> jobCaps, String builderName, String fieldName) {
String builderName,
String fieldName,
String timeZone) {
List<String> incompatibleTimeZones = timeZone == null ? Collections.emptyList() : new ArrayList<>();
List<String> rewrittenFieldNames = jobCaps.stream() List<String> rewrittenFieldNames = jobCaps.stream()
// We only care about job caps that have the query's target field // We only care about job caps that have the query's target field
.filter(caps -> caps.getFieldCaps().keySet().contains(fieldName)) .filter(caps -> caps.getFieldCaps().keySet().contains(fieldName))
@ -335,17 +325,7 @@ public class TransportRollupSearchAction extends TransportAction<SearchRequest,
// For now, we only allow filtering on grouping fields // For now, we only allow filtering on grouping fields
.filter(agg -> { .filter(agg -> {
String type = (String)agg.get(RollupField.AGG); String type = (String)agg.get(RollupField.AGG);
// make sure it's one of the three groups
// If the cap is for a date_histo, and the query is a range, the timezones need to match
if (type.equals(DateHistogramAggregationBuilder.NAME) && timeZone != null) {
boolean matchingTZ = ((String)agg.get(DateHistogramGroupConfig.TIME_ZONE))
.equalsIgnoreCase(timeZone);
if (matchingTZ == false) {
incompatibleTimeZones.add((String)agg.get(DateHistogramGroupConfig.TIME_ZONE));
}
return matchingTZ;
}
// Otherwise just make sure it's one of the three groups
return type.equals(TermsAggregationBuilder.NAME) return type.equals(TermsAggregationBuilder.NAME)
|| type.equals(DateHistogramAggregationBuilder.NAME) || type.equals(DateHistogramAggregationBuilder.NAME)
|| type.equals(HistogramAggregationBuilder.NAME); || type.equals(HistogramAggregationBuilder.NAME);
@ -363,14 +343,8 @@ public class TransportRollupSearchAction extends TransportAction<SearchRequest,
.distinct() .distinct()
.collect(ArrayList::new, List::addAll, List::addAll); .collect(ArrayList::new, List::addAll, List::addAll);
if (rewrittenFieldNames.isEmpty()) { if (rewrittenFieldNames.isEmpty()) {
if (incompatibleTimeZones.isEmpty()) { throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName
throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName
+ "] query is not available in selected rollup indices, cannot query."); + "] query is not available in selected rollup indices, cannot query.");
} else {
throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName
+ "] query was found in rollup indices, but requested timezone is not compatible. Options include: "
+ incompatibleTimeZones);
}
} else if (rewrittenFieldNames.size() > 1) { } else if (rewrittenFieldNames.size() > 1) {
throw new IllegalArgumentException("Ambiguous field name resolution when mapping to rolled fields. Field name [" + throw new IllegalArgumentException("Ambiguous field name resolution when mapping to rolled fields. Field name [" +
fieldName + "] was mapped to: [" + Strings.collectionToDelimitedString(rewrittenFieldNames, ",") + "]."); fieldName + "] was mapped to: [" + Strings.collectionToDelimitedString(rewrittenFieldNames, ",") + "].");

View File

@ -140,16 +140,15 @@ public class SearchActionTests extends ESTestCase {
assertThat(((RangeQueryBuilder)rewritten).fieldName(), equalTo("foo.date_histogram.timestamp")); assertThat(((RangeQueryBuilder)rewritten).fieldName(), equalTo("foo.date_histogram.timestamp"));
} }
public void testRangeWrongTZ() { public void testRangeDifferentTZ() {
final GroupConfig groupConfig = new GroupConfig(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, "UTC")); final GroupConfig groupConfig = new GroupConfig(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, "UTC"));
final RollupJobConfig config = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, groupConfig, emptyList(), null); final RollupJobConfig config = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, groupConfig, emptyList(), null);
RollupJobCaps cap = new RollupJobCaps(config); RollupJobCaps cap = new RollupJobCaps(config);
Set<RollupJobCaps> caps = new HashSet<>(); Set<RollupJobCaps> caps = new HashSet<>();
caps.add(cap); caps.add(cap);
Exception e = expectThrows(IllegalArgumentException.class, QueryBuilder rewritten = TransportRollupSearchAction.rewriteQuery(new RangeQueryBuilder("foo").gt(1).timeZone("CET"), caps);
() -> TransportRollupSearchAction.rewriteQuery(new RangeQueryBuilder("foo").gt(1).timeZone("CET"), caps)); assertThat(rewritten, instanceOf(RangeQueryBuilder.class));
assertThat(e.getMessage(), equalTo("Field [foo] in [range] query was found in rollup indices, but requested timezone is not " + assertThat(((RangeQueryBuilder)rewritten).fieldName(), equalTo("foo.date_histogram.timestamp"));
"compatible. Options include: [UTC]"));
} }
public void testTermQuery() { public void testTermQuery() {