Skip to content

Commit

Permalink
[Rollup] Validate timezones based on rules not string comparision (#3…
Browse files Browse the repository at this point in the history 
…6237)

The date_histogram internally converts obsolete timezones (such as
"Canada/Mountain") into their modern equivalent ("America/Edmonton").
But rollup just stored the TZ as provided by the user.

When checking the TZ for query validation we used a string comparison,
which would fail due to the date_histo's upgrading behavior.

Instead, we should convert both to a TimeZone object and check if their
rules are compatible.
  • Loading branch information
@polyfractal
polyfractal committed Apr 17, 2019
1 parent 4d96419 commit 7e62ff2
Show file tree
Hide file tree
Showing 15 changed files with 645 additions and 158 deletions.
121 changes: 121 additions & 0 deletions server/src/main/java/org/elasticsearch/common/time/DateUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,127 @@ public static DateTimeZone zoneIdToDateTimeZone(ZoneId zoneId) {
DEPRECATED_SHORT_TZ_IDS = tzs.keySet();
}

// Map of deprecated timezones and their recommended new counterpart
public static final Map<String, String> DEPRECATED_LONG_TIMEZONES;
static {
Map<String, String> tzs = new HashMap<>();
tzs.put("Africa/Asmera","Africa/Nairobi");
tzs.put("Africa/Timbuktu","Africa/Abidjan");
tzs.put("America/Argentina/ComodRivadavia","America/Argentina/Catamarca");
tzs.put("America/Atka","America/Adak");
tzs.put("America/Buenos_Aires","America/Argentina/Buenos_Aires");
tzs.put("America/Catamarca","America/Argentina/Catamarca");
tzs.put("America/Coral_Harbour","America/Atikokan");
tzs.put("America/Cordoba","America/Argentina/Cordoba");
tzs.put("America/Ensenada","America/Tijuana");
tzs.put("America/Fort_Wayne","America/Indiana/Indianapolis");
tzs.put("America/Indianapolis","America/Indiana/Indianapolis");
tzs.put("America/Jujuy","America/Argentina/Jujuy");
tzs.put("America/Knox_IN","America/Indiana/Knox");
tzs.put("America/Louisville","America/Kentucky/Louisville");
tzs.put("America/Mendoza","America/Argentina/Mendoza");
tzs.put("America/Montreal","America/Toronto");
tzs.put("America/Porto_Acre","America/Rio_Branco");
tzs.put("America/Rosario","America/Argentina/Cordoba");
tzs.put("America/Santa_Isabel","America/Tijuana");
tzs.put("America/Shiprock","America/Denver");
tzs.put("America/Virgin","America/Port_of_Spain");
tzs.put("Antarctica/South_Pole","Pacific/Auckland");
tzs.put("Asia/Ashkhabad","Asia/Ashgabat");
tzs.put("Asia/Calcutta","Asia/Kolkata");
tzs.put("Asia/Chongqing","Asia/Shanghai");
tzs.put("Asia/Chungking","Asia/Shanghai");
tzs.put("Asia/Dacca","Asia/Dhaka");
tzs.put("Asia/Harbin","Asia/Shanghai");
tzs.put("Asia/Kashgar","Asia/Urumqi");
tzs.put("Asia/Katmandu","Asia/Kathmandu");
tzs.put("Asia/Macao","Asia/Macau");
tzs.put("Asia/Rangoon","Asia/Yangon");
tzs.put("Asia/Saigon","Asia/Ho_Chi_Minh");
tzs.put("Asia/Tel_Aviv","Asia/Jerusalem");
tzs.put("Asia/Thimbu","Asia/Thimphu");
tzs.put("Asia/Ujung_Pandang","Asia/Makassar");
tzs.put("Asia/Ulan_Bator","Asia/Ulaanbaatar");
tzs.put("Atlantic/Faeroe","Atlantic/Faroe");
tzs.put("Atlantic/Jan_Mayen","Europe/Oslo");
tzs.put("Australia/ACT","Australia/Sydney");
tzs.put("Australia/Canberra","Australia/Sydney");
tzs.put("Australia/LHI","Australia/Lord_Howe");
tzs.put("Australia/NSW","Australia/Sydney");
tzs.put("Australia/North","Australia/Darwin");
tzs.put("Australia/Queensland","Australia/Brisbane");
tzs.put("Australia/South","Australia/Adelaide");
tzs.put("Australia/Tasmania","Australia/Hobart");
tzs.put("Australia/Victoria","Australia/Melbourne");
tzs.put("Australia/West","Australia/Perth");
tzs.put("Australia/Yancowinna","Australia/Broken_Hill");
tzs.put("Brazil/Acre","America/Rio_Branco");
tzs.put("Brazil/DeNoronha","America/Noronha");
tzs.put("Brazil/East","America/Sao_Paulo");
tzs.put("Brazil/West","America/Manaus");
tzs.put("Canada/Atlantic","America/Halifax");
tzs.put("Canada/Central","America/Winnipeg");
tzs.put("Canada/East-Saskatchewan","America/Regina");
tzs.put("Canada/Eastern","America/Toronto");
tzs.put("Canada/Mountain","America/Edmonton");
tzs.put("Canada/Newfoundland","America/St_Johns");
tzs.put("Canada/Pacific","America/Vancouver");
tzs.put("Canada/Yukon","America/Whitehorse");
tzs.put("Chile/Continental","America/Santiago");
tzs.put("Chile/EasterIsland","Pacific/Easter");
tzs.put("Cuba","America/Havana");
tzs.put("Egypt","Africa/Cairo");
tzs.put("Eire","Europe/Dublin");
tzs.put("Europe/Belfast","Europe/London");
tzs.put("Europe/Tiraspol","Europe/Chisinau");
tzs.put("GB","Europe/London");
tzs.put("GB-Eire","Europe/London");
tzs.put("Greenwich","Etc/GMT");
tzs.put("Hongkong","Asia/Hong_Kong");
tzs.put("Iceland","Atlantic/Reykjavik");
tzs.put("Iran","Asia/Tehran");
tzs.put("Israel","Asia/Jerusalem");
tzs.put("Jamaica","America/Jamaica");
tzs.put("Japan","Asia/Tokyo");
tzs.put("Kwajalein","Pacific/Kwajalein");
tzs.put("Libya","Africa/Tripoli");
tzs.put("Mexico/BajaNorte","America/Tijuana");
tzs.put("Mexico/BajaSur","America/Mazatlan");
tzs.put("Mexico/General","America/Mexico_City");
tzs.put("NZ","Pacific/Auckland");
tzs.put("NZ-CHAT","Pacific/Chatham");
tzs.put("Navajo","America/Denver");
tzs.put("PRC","Asia/Shanghai");
tzs.put("Pacific/Johnston","Pacific/Honolulu");
tzs.put("Pacific/Ponape","Pacific/Pohnpei");
tzs.put("Pacific/Samoa","Pacific/Pago_Pago");
tzs.put("Pacific/Truk","Pacific/Chuuk");
tzs.put("Pacific/Yap","Pacific/Chuuk");
tzs.put("Poland","Europe/Warsaw");
tzs.put("Portugal","Europe/Lisbon");
tzs.put("ROC","Asia/Taipei");
tzs.put("ROK","Asia/Seoul");
tzs.put("Singapore","Asia/Singapore");
tzs.put("Turkey","Europe/Istanbul");
tzs.put("UCT","Etc/UCT");
tzs.put("US/Alaska","America/Anchorage");
tzs.put("US/Aleutian","America/Adak");
tzs.put("US/Arizona","America/Phoenix");
tzs.put("US/Central","America/Chicago");
tzs.put("US/East-Indiana","America/Indiana/Indianapolis");
tzs.put("US/Eastern","America/New_York");
tzs.put("US/Hawaii","Pacific/Honolulu");
tzs.put("US/Indiana-Starke","America/Indiana/Knox");
tzs.put("US/Michigan","America/Detroit");
tzs.put("US/Mountain","America/Denver");
tzs.put("US/Pacific","America/Los_Angeles");
tzs.put("US/Samoa","Pacific/Pago_Pago");
tzs.put("Universal","Etc/UTC");
tzs.put("W-SU","Europe/Moscow");
tzs.put("Zulu","Etc/UTC");
DEPRECATED_LONG_TIMEZONES = Collections.unmodifiableMap(tzs);
}

public static ZoneId dateTimeZoneToZoneId(DateTimeZone timeZone) {
if (timeZone == null) {
return null;
Expand Down
10 changes: 6 additions & 4 deletions .../core/src/main/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import java.io.IOException;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.Map;
import java.util.Objects;

Expand Down Expand Up @@ -52,7 +53,8 @@ public class DateHistogramGroupConfig implements Writeable, ToXContentObject {
private static final String FIELD = "field";
public static final String TIME_ZONE = "time_zone";
public static final String DELAY = "delay";
private static final String DEFAULT_TIMEZONE = "UTC";
public static final String DEFAULT_TIMEZONE = "UTC";
public static final ZoneId DEFAULT_ZONEID_TIMEZONE = ZoneOffset.UTC;
private static final ConstructingObjectParser<DateHistogramGroupConfig, Void> PARSER;
static {
PARSER = new ConstructingObjectParser<>(NAME, a ->
Expand Down Expand Up @@ -210,12 +212,12 @@ public boolean equals(final Object other) {
return Objects.equals(interval, that.interval)
&& Objects.equals(field, that.field)
&& Objects.equals(delay, that.delay)
&& Objects.equals(timeZone, that.timeZone);
&& ZoneId.of(timeZone, ZoneId.SHORT_IDS).getRules().equals(ZoneId.of(that.timeZone, ZoneId.SHORT_IDS).getRules());
}

@Override
public int hashCode() {
return Objects.hash(interval, field, delay, timeZone);
return Objects.hash(interval, field, delay, ZoneId.of(timeZone));
}

@Override
Expand All @@ -235,7 +237,7 @@ private static Rounding createRounding(final String expr, final String timeZone)
} else {
rounding = new Rounding.Builder(TimeValue.parseTimeValue(expr, "createRounding"));
}
rounding.timeZone(ZoneId.of(timeZone));
rounding.timeZone(ZoneId.of(timeZone, ZoneId.SHORT_IDS));
return rounding.build();
}
}
4 changes: 3 additions & 1 deletion ...org/elasticsearch/xpack/ml/datafeed/extractor/aggregation/RollupDataExtractorFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
import org.elasticsearch.xpack.ml.datafeed.extractor.DataExtractorFactory;

import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand Down Expand Up @@ -124,7 +126,7 @@ private static boolean validInterval(long datafeedInterval, ParsedRollupCaps rol
if (rollupJobGroupConfig.hasDatehistogram() == false) {
return false;
}
if ("UTC".equalsIgnoreCase(rollupJobGroupConfig.getTimezone()) == false) {
if (ZoneId.of(rollupJobGroupConfig.getTimezone()).getRules().equals(ZoneOffset.UTC.getRules()) == false) {
return false;
}
try {
Expand Down
9 changes: 6 additions & 3 deletions .../plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps;
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;

import java.time.ZoneId;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashSet;
Expand Down Expand Up @@ -96,11 +97,13 @@ private static void checkDateHisto(DateHistogramAggregationBuilder source, List<
if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
DateHistogramInterval interval = new DateHistogramInterval((String)agg.get(RollupField.INTERVAL));

String thisTimezone = (String)agg.get(DateHistogramGroupConfig.TIME_ZONE);
String sourceTimeZone = source.timeZone() == null ? "UTC" : source.timeZone().toString();
ZoneId thisTimezone = ZoneId.of(((String) agg.get(DateHistogramGroupConfig.TIME_ZONE)), ZoneId.SHORT_IDS);
ZoneId sourceTimeZone = source.timeZone() == null
? DateHistogramGroupConfig.DEFAULT_ZONEID_TIMEZONE
: ZoneId.of(source.timeZone().toString(), ZoneId.SHORT_IDS);

// Ensure we are working on the same timezone
if (thisTimezone.equalsIgnoreCase(sourceTimeZone) == false) {
if (thisTimezone.getRules().equals(sourceTimeZone.getRules()) == false) {
continue;
}
if (source.dateHistogramInterval() != null) {
Expand Down
52 changes: 14 additions & 38 deletions ...k/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupRequestTranslator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.TermQueryBuilder;
import org.elasticsearch.search.aggregations.AggregationBuilder;
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
Expand All @@ -22,8 +20,8 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.xpack.core.rollup.RollupField;
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
import org.joda.time.DateTimeZone;

import java.time.ZoneId;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand All @@ -47,7 +45,7 @@
* }</pre>
*
*
* The only publicly "consumable" API is {@link #translateAggregation(AggregationBuilder, List, NamedWriteableRegistry)}.
* The only publicly "consumable" API is {@link #translateAggregation(AggregationBuilder, NamedWriteableRegistry)}.
*/
public class RollupRequestTranslator {

Expand Down Expand Up @@ -116,26 +114,22 @@ public class RollupRequestTranslator {
* relevant method below.
*
* @param source The source aggregation to translate into rollup-enabled version
* @param filterConditions A list used to track any filter conditions that sub-aggs may
* require.
* @param registry Registry containing the various aggregations so that we can easily
* deserialize into a stream for cloning
* @return Returns the fully translated aggregation tree. Note that it returns a list instead
* of a single AggBuilder, since some aggregations (e.g. avg) may result in two
* translated aggs (sum + count)
*/
public static List<AggregationBuilder> translateAggregation(AggregationBuilder source,
List<QueryBuilder> filterConditions,
NamedWriteableRegistry registry) {
public static List<AggregationBuilder> translateAggregation(AggregationBuilder source, NamedWriteableRegistry registry) {

if (source.getWriteableName().equals(DateHistogramAggregationBuilder.NAME)) {
return translateDateHistogram((DateHistogramAggregationBuilder) source, filterConditions, registry);
return translateDateHistogram((DateHistogramAggregationBuilder) source, registry);
} else if (source.getWriteableName().equals(HistogramAggregationBuilder.NAME)) {
return translateHistogram((HistogramAggregationBuilder) source, filterConditions, registry);
return translateHistogram((HistogramAggregationBuilder) source, registry);
} else if (RollupField.SUPPORTED_METRICS.contains(source.getWriteableName())) {
return translateVSLeaf((ValuesSourceAggregationBuilder.LeafOnly)source, registry);
} else if (source.getWriteableName().equals(TermsAggregationBuilder.NAME)) {
return translateTerms((TermsAggregationBuilder)source, filterConditions, registry);
return translateTerms((TermsAggregationBuilder)source, registry);
} else {
throw new IllegalArgumentException("Unable to translate aggregation tree into Rollup. Aggregation ["
+ source.getName() + "] is of type [" + source.getClass().getSimpleName() + "] which is " +
Expand Down Expand Up @@ -195,22 +189,13 @@ public static List<AggregationBuilder> translateAggregation(AggregationBuilder s
* <li>Field: `{timestamp field}.date_histogram._count`</li>
* </ul>
* </li>
* <li>Add a filter condition:</li>
* <li>
* <ul>
* <li>Query type: TermQuery</li>
* <li>Field: `{timestamp_field}.date_histogram.interval`</li>
* <li>Value: `{source interval}`</li>
* </ul>
* </li>
* </ul>
*
*/
private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggregationBuilder source,
List<QueryBuilder> filterConditions,
NamedWriteableRegistry registry) {

return translateVSAggBuilder(source, filterConditions, registry, () -> {
return translateVSAggBuilder(source, registry, () -> {
DateHistogramAggregationBuilder rolledDateHisto
= new DateHistogramAggregationBuilder(source.getName());

Expand All @@ -220,13 +205,9 @@ private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggr
rolledDateHisto.interval(source.interval());
}

String timezone = source.timeZone() == null ? DateTimeZone.UTC.toString() : source.timeZone().toString();
filterConditions.add(new TermQueryBuilder(RollupField.formatFieldName(source,
DateHistogramGroupConfig.TIME_ZONE), timezone));
ZoneId timeZone = source.timeZone() == null ? DateHistogramGroupConfig.DEFAULT_ZONEID_TIMEZONE : source.timeZone();
rolledDateHisto.timeZone(timeZone);

if (source.timeZone() != null) {
rolledDateHisto.timeZone(source.timeZone());
}
rolledDateHisto.offset(source.offset());
if (source.extendedBounds() != null) {
rolledDateHisto.extendedBounds(source.extendedBounds());
Expand All @@ -248,14 +229,13 @@ private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggr
* Notably, it adds a Sum metric to calculate the doc_count in each bucket.
*
* Conventions are identical to a date_histogram (excepting date-specific details), so see
* {@link #translateDateHistogram(DateHistogramAggregationBuilder, List, NamedWriteableRegistry)} for
* {@link #translateDateHistogram(DateHistogramAggregationBuilder, NamedWriteableRegistry)} for
* a complete list of conventions, examples, etc
*/
private static List<AggregationBuilder> translateHistogram(HistogramAggregationBuilder source,
List<QueryBuilder> filterConditions,
NamedWriteableRegistry registry) {

return translateVSAggBuilder(source, filterConditions, registry, () -> {
return translateVSAggBuilder(source, registry, () -> {
HistogramAggregationBuilder rolledHisto
= new HistogramAggregationBuilder(source.getName());

Expand Down Expand Up @@ -328,10 +308,9 @@ private static List<AggregationBuilder> translateHistogram(HistogramAggregationB
*
*/
private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder source,
List<QueryBuilder> filterConditions,
NamedWriteableRegistry registry) {

return translateVSAggBuilder(source, filterConditions, registry, () -> {
return translateVSAggBuilder(source, registry, () -> {
TermsAggregationBuilder rolledTerms
= new TermsAggregationBuilder(source.getName(), source.valueType());
rolledTerms.field(RollupField.formatFieldName(source, RollupField.VALUE));
Expand Down Expand Up @@ -359,8 +338,6 @@ private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder s
* ValueSourceBuilder. This method is called by all the agg-specific methods (e.g. translateDateHistogram())
*
* @param source The source aggregation that we wish to translate
* @param filterConditions A list of existing filter conditions, in case we need to add some
* for this particular agg
* @param registry Named registry for serializing leaf metrics. Not actually used by this method,
* but is passed downwards for leaf usage
* @param factory A factory closure that generates a new shallow clone of the `source`. E.g. if `source` is
Expand All @@ -371,15 +348,14 @@ private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder s
* @return the translated multi-bucket ValueSourceAggBuilder
*/
private static <T extends ValuesSourceAggregationBuilder> List<AggregationBuilder>
translateVSAggBuilder(ValuesSourceAggregationBuilder source, List<QueryBuilder> filterConditions,
NamedWriteableRegistry registry, Supplier<T> factory) {
translateVSAggBuilder(ValuesSourceAggregationBuilder source, NamedWriteableRegistry registry, Supplier<T> factory) {

T rolled = factory.get();

// Translate all subaggs and add to the newly translated agg
// NOTE: using for loop instead of stream because compiler explodes with a bug :/
for (AggregationBuilder subAgg : source.getSubAggregations()) {
List<AggregationBuilder> translated = translateAggregation(subAgg, filterConditions, registry);
List<AggregationBuilder> translated = translateAggregation(subAgg, registry);
for (AggregationBuilder t : translated) {
rolled.subAggregation(t);
}
Expand Down
Loading

0 comments on commit 7e62ff2

Please sign in to comment.